[Libvir] Supporting new Xen 3.0.3 blktap backend for file devices

[Daniel Veillard]

On Thu, Oct 05, 2006 at 07:23:15PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 27, 2006 at 06:36:38PM +0100, Daniel P. Berrange wrote:
> > Ok, looks like we have total agreement. I'll knock up a patch implementing
> > option #4 and post it for review.
> 
> Attached is a patch which implements option 4. If no <driver> block is
> supplied, then we continue existing behavior of using file: for file backed
> disks, and phy: for block backed disks. Any of the following are then valid:
> 
>   <driver name='file'/>                ->     file:
>   <driver name='phy'/>                 ->     phy:
>   <driver name='tap'/>                 ->     tap:aio:
>   <driver name='tap' type='aio'/>      ->     tap:aio:
>   <driver name='tap' type='qcow'/>     ->     tap:qcow:
> 
> The only supported names are 'file', 'phy' & 'tap'. If the name is 'tap'
> then it is valid to use an additional 'type' attributte.  We don't do
> any checking on cotents of 'type' attribute, it is just passed straight
> through to xend, since the blktap driver has a wide variety of types
> available. When fetching XML from libvirt  you are now guarenteed to
> be given a <driver> block in each disk.

  Okay,

> The patch was rather tedious, because not only did the blktap stuff change
> the prefix used on file paths in the (uname) SEXPR block, but it actually
> changed the entire enclosing block from (vbd ...) to (tap ...) for some
> crazy reason.

  Oh well ... at some point we will need to look at the new interfaces for
xend too, but one thing at a time :-)

> I'm not attaching them here because it would bloat the patch, but I've written
> a tonne more testfiles for the xml <-> sexpr conversions to validate the 
> patch is operating correctly.

  Great !

> Index: src/xend_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.c,v
> retrieving revision 1.66
> diff -u -r1.66 xend_internal.c
> --- src/xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
> +++ src/xend_internal.c	5 Oct 2006 18:12:33 -0000
> @@ -1499,7 +1499,7 @@
>  	for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) {
>  	    if (((tmp[j] >= '0') && (tmp[j] <= '9')) ||
>  	        ((tmp[j] >= 'a') && (tmp[j] <= 'f')))
> -		compact[i++] = tmp[j];
> +            compact[i++] = tmp[j];

  maybe we should just add the full set of { } for the innner constructs 
too if reformatting.

>  	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
>  	        compact[i++] = tmp[j] + 'a' - 'A';
>  	}
> @@ -1546,95 +1546,116 @@
> +            drvName = malloc((offset-src)+1);

  I agree that if we OOM there it's gonna be messy anyway but let's catch
NULL returns on allocs as much as possible

> +            strncpy(drvName, src, (offset-src));
> +            drvName[offset-src] = '\0';
> +
> +            src = offset + 1;
> +
> +            if (!strcmp(drvName, "tap")) {
> +                offset = strchr(src, ':');
> +                if (!offset)
> +                    goto bad_parse;
> +
> +                drvType = malloc((offset-src)+1);

  Same here. If failing a libvirt error and going to bad_parse should be
sufficient I guess.

> +                    } else if (!strcmp(offset, ":disk")) {
> +                        /* defualt anyway */

  /* default */ :-)
> +            } else if ((drvName == NULL) &&
> +                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
> +                drvName = xmlGetProp(cur, BAD_CAST "name");

  testing for NULL would be good, if the attribute is missing we should
not crash

> +                if (!strcmp((const char *)drvName, "tap"))
> +                    drvType = xmlGetProp(cur, BAD_CAST "type");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
>                  ro = 1;
>              }
> @@ -972,14 +979,14 @@
>      /* Xend (all versions) put the floppy device config
>       * under the hvm (image (os)) block
>       */
> -    if (hvm && 
> +    if (hvm &&
>          device &&
>          !strcmp((const char *)device, "floppy")) {
>          return 0;
>      }
>  
>      /* Xend <= 3.0.2 doesn't include cdrom config here */
> -    if (hvm && 
> +    if (hvm &&
>          device &&
>          !strcmp((const char *)device, "cdrom")) {
>          if (xendConfigVersion == 1)
> @@ -990,7 +997,14 @@
>  
>  
>      virBufferAdd(buf, "(device ", 8);
> -    virBufferAdd(buf, "(vbd ", 5);
> +    /* Why, oh why did this need to change as well as the
> +       specifying tap in the (uname..) block ??!!?! Crazy
> +       Xen formats :-( */

  I undestand the frustration, but 3 years from now the comment will lack
relevance, unless you give the full picture :-)

    thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/