[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Fri, Oct 06, 2006 at 04:44:17AM -0400, Daniel Veillard wrote:
> 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.
> 

Attached is an update version of the patch addressing the issues mentioned.


Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.66
diff -u -r1.66 xend_internal.c
--- xend_internal.c	29 Sep 2006 12:00:58 -0000	1.66
+++ xend_internal.c	6 Oct 2006 13:52:03 -0000
@@ -1451,7 +1451,7 @@
 
 /**
  * xend_parse_sexp_desc:
- * @domain: the domain associated with the XML
+ * @conn: the connection associated with the XML
  * @root: the root of the parsed S-Expression
  *
  * Parse the xend sexp description and turn it into the XML format similar
@@ -1487,7 +1487,7 @@
 
     tmp = sexpr_node(root, "domain/name");
     if (tmp == NULL) {
-        virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+        virXendError(conn, VIR_ERR_INTERNAL_ERROR,
                      _("domain information incomplete, missing name"));
         goto error;
     }
@@ -1498,10 +1498,11 @@
 	int i, j;
 	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];
-	    else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
+	        ((tmp[j] >= 'a') && (tmp[j] <= 'f'))) {
+            compact[i++] = tmp[j];
+        } else if ((tmp[j] >= 'A') && (tmp[j] <= 'F')) {
 	        compact[i++] = tmp[j] + 'a' - 'A';
+        }
 	}
 	compact[i] = 0;
 	if (i > 0)
@@ -1509,7 +1510,7 @@
     }
     tmp = sexpr_node(root, "domain/bootloader");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);
+        virBufferVSprintf(&buf, "  <bootloader>%s</bootloader>\n", tmp);
 
     if (sexpr_lookup(root, "domain/image")) {
         hvm = sexpr_lookup(root, "domain/image/hvm") ? 1 : 0;
@@ -1522,13 +1523,13 @@
                       sexpr_int(root, "domain/vcpus"));
     tmp = sexpr_node(root, "domain/on_poweroff");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_poweroff>%s</on_poweroff>\n", tmp);
     tmp = sexpr_node(root, "domain/on_reboot");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_reboot>%s</on_reboot>\n", tmp);
     tmp = sexpr_node(root, "domain/on_crash");
     if (tmp != NULL)
-	virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);
+        virBufferVSprintf(&buf, "  <on_crash>%s</on_crash>\n", tmp);
 
     if (hvm) {
         virBufferAdd(&buf, "  <features>\n", 13);
@@ -1546,105 +1547,150 @@
     /* in case of HVM we have devices emulation */
     tmp = sexpr_node(root, "domain/image/hvm/device_model");
     if ((tmp != NULL) && (tmp[0] != 0))
-	virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);
+        virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);
 
     for (cur = root; cur->kind == SEXPR_CONS; cur = cur->cdr) {
         node = cur->car;
-        if (sexpr_lookup(node, "device/vbd")) {
-            tmp = sexpr_node(node, "device/vbd/uname");
-            if (tmp == NULL)
-                continue;
-            if (!memcmp(tmp, "file:", 5)) {
-                int cdrom = 0;
-                const char *src = tmp+5;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
-                }
+        /* Normally disks are in a (device (vbd ...)) block
+           but blktap disks ended up in a differently named
+           (device (tap ....)) block.... */
+        if (sexpr_lookup(node, "device/vbd") ||
+            sexpr_lookup(node, "device/tap")) {
+            char *offset;
+            int isBlock = 0;
+            int cdrom = 0;
+            char *drvName = NULL;
+            char *drvType = NULL;
+            const char *src = NULL;
+            const char *dst = NULL;
+            const char *mode = NULL;
+
+            /* Again dealing with (vbd...) vs (tap ...) differences */
+            if (sexpr_lookup(node, "device/vbd")) {
+                src = sexpr_node(node, "device/vbd/uname");
+                dst = sexpr_node(node, "device/vbd/dev");
+                mode = sexpr_node(node, "device/vbd/mode");
+            } else {
+                src = sexpr_node(node, "device/tap/uname");
+                dst = sexpr_node(node, "device/tap/dev");
+                mode = sexpr_node(node, "device/tap/mode");
+            }
 
-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style disk config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
-                    }
+            if (src == NULL) {
+                virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no src"));
+                goto bad_parse;
+            }
+
+            if (dst == NULL) {
+                virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no dev"));
+                goto bad_parse;
+            }
+
+
+            offset = strchr(src, ':');
+            if (!offset) {
+                virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                             _("cannot parse vbd filename, missing driver name"));
+                goto bad_parse;
+            }
+
+            drvName = malloc((offset-src)+1);
+            if (!drvName) {
+                virXendError(conn, VIR_ERR_NO_MEMORY,
+                             _("allocate new buffer"));
+                goto bad_parse;
+            }
+            strncpy(drvName, src, (offset-src));
+            drvName[offset-src] = '\0';
+
+            src = offset + 1;
+
+            if (!strcmp(drvName, "tap")) {
+                offset = strchr(src, ':');
+                if (!offset) {
+                    virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                                 _("cannot parse vbd filename, missing driver type"));
+                    goto bad_parse;
                 }
 
-                virBufferVSprintf(&buf, "    <disk type='file' device='%s'>\n", cdrom ? "cdrom" : "disk");
-                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
-            } else if (!memcmp(tmp, "phy:", 4)) {
-                int cdrom = 0;
-                const char *src = tmp+4;
-                const char *dst = sexpr_node(node, "device/vbd/dev");
-
-                if (dst == NULL) {
-                    virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no dev"));
-                    goto error;
+                drvType = malloc((offset-src)+1);
+                if (!drvType) {
+                    virXendError(conn, VIR_ERR_NO_MEMORY,
+                                 _("allocate new buffer"));
+                    goto bad_parse;
                 }
+                strncpy(drvType, src, (offset-src));
+                drvType[offset-src] = '\0';
+                src = offset + 1;
+                /* Its possible to use blktap driver for block devs
+                   too, but kinda pointless because blkback is better,
+                   so we assume common case here. If blktap becomes
+                   omnipotent, we can revisit this, perhaps stat()'ing
+                   the src file in question */
+                isBlock = 0;
+            } else if (!strcmp(drvName, "phy")) {
+                isBlock = 1;
+            } else if (!strcmp(drvName, "file")) {
+                isBlock = 0;
+            }
+
+            if (!strncmp(dst, "ioemu:", 6))
+                dst += 6;
 
-                if (!strncmp(dst, "ioemu:", 6)) 
-                    dst += 6;
-                /* New style cdrom config from Xen >= 3.0.3 */
-                if (xendConfigVersion > 1) {
-                    char *offset = rindex(dst, ':');
-                    if (offset) {
-                        if (!strcmp(offset, ":cdrom")) {
-                            cdrom = 1;
-                        } else if (!strcmp(offset, ":disk")) {
-                            /* defualt anyway */
-                        } else {
-                            /* Unknown, lets pretend its a disk */
-                        }
-                        offset[0] = '\0';
+            /* New style disk config from Xen >= 3.0.3 */
+            if (xendConfigVersion > 1) {
+                offset = rindex(dst, ':');
+                if (offset) {
+                    if (!strcmp(offset, ":cdrom")) {
+                        cdrom = 1;
+                    } else if (!strcmp(offset, ":disk")) {
+                        /* The default anyway */
+                    } else {
+                        /* Unknown, lets pretend its a disk too */
                     }
+                    offset[0] = '\0';
                 }
+            }
 
-                virBufferVSprintf(&buf, "    <disk type='block' device='%s'>\n", cdrom ? "cdrom" : "disk");
+            virBufferVSprintf(&buf, "    <disk type='%s' device='%s'>\n",
+                              isBlock ? "block" : "file",
+                              cdrom ? "cdrom" : "disk");
+            if (drvType) {
+                virBufferVSprintf(&buf, "      <driver name='%s' type='%s'/>\n", drvName, drvType);
+            } else {
+                virBufferVSprintf(&buf, "      <driver name='%s'/>\n", drvName);
+            }
+            if (isBlock) {
                 virBufferVSprintf(&buf, "      <source dev='%s'/>\n", src);
-                virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-                tmp = sexpr_node(node, "device/vbd/mode");
-                /* XXX should we force mode == r, if cdrom==1, or assume
-                   xend has already done this ? */
-                if ((tmp != NULL) && (!strcmp(tmp, "r")))
-                    virBufferVSprintf(&buf, "      <readonly/>\n");
-                virBufferAdd(&buf, "    </disk>\n", 12);
             } else {
-                char serial[1000];
+                virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
+            }
+            virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
 
-                TODO sexpr2string(node, serial, 1000);
-                virBufferVSprintf(&buf, "<!-- Failed to parse %s -->\n",
-                                  serial);
-            TODO}
+
+            /* XXX should we force mode == r, if cdrom==1, or assume
+               xend has already done this ? */
+            if ((mode != NULL) && (!strcmp(mode, "r")))
+                virBufferVSprintf(&buf, "      <readonly/>\n");
+            virBufferAdd(&buf, "    </disk>\n", 12);
+
+            bad_parse:
+            if (drvName)
+                free(drvName);
+            if (drvType)
+                free(drvType);
         } else if (sexpr_lookup(node, "device/vif")) {
-	    const char *tmp2;
+            const char *tmp2;
 
             tmp = sexpr_node(node, "device/vif/bridge");
-	    tmp2 = sexpr_node(node, "device/vif/script");
+            tmp2 = sexpr_node(node, "device/vif/script");
             if ((tmp != NULL) || (strstr(tmp2, "bridge"))) {
                 virBufferVSprintf(&buf, "    <interface type='bridge'>\n");
-		if (tmp != NULL)
-		    virBufferVSprintf(&buf, "      <source bridge='%s'/>\n",
-				      tmp);
+                if (tmp != NULL)
+                    virBufferVSprintf(&buf, "      <source bridge='%s'/>\n",
+                                      tmp);
                 tmp = sexpr_node(node, "device/vif/vifname");
                 if (tmp != NULL)
                     virBufferVSprintf(&buf, "      <target dev='%s'/>\n",
@@ -1688,10 +1734,11 @@
         }
 
         /* Old style cdrom config from Xen <= 3.0.2 */
-	if (xendConfigVersion == 1) {
+        if (xendConfigVersion == 1) {
             tmp = sexpr_node(root, "domain/image/hvm/cdrom");
             if ((tmp != NULL) && (tmp[0] != 0)) {
                 virBufferAdd(&buf, "    <disk type='file' device='cdrom'>\n", 38);
+                virBufferAdd(&buf, "      <driver name='file'/>\n", 28);
                 virBufferVSprintf(&buf, "      <source file='%s'/>\n", tmp);
                 virBufferAdd(&buf, "      <target dev='hdc'/>\n", 26);
                 virBufferAdd(&buf, "      <readonly/>\n", 18);
@@ -1699,24 +1746,24 @@
             }
         }
     }
-        
+
     /* Graphics device */
     tmp = sexpr_fmt_node(root, "domain/image/%s/vnc", hvm ? "hvm" : "linux");
     if (tmp != NULL) {
         if (tmp[0] == '1') {
             int port = xenStoreDomainGetVNCPort(conn, domid);
-            if (port == -1) 
+            if (port == -1)
                 port = 5900 + domid;
             virBufferVSprintf(&buf, "    <graphics type='vnc' port='%d'/>\n", port);
         }
     }
-        
+
     tmp = sexpr_fmt_node(root, "domain/image/%s/sdl", hvm ? "hvm" : "linux");
     if (tmp != NULL) {
         if (tmp[0] == '1')
             virBufferAdd(&buf, "    <graphics type='sdl'/>\n", 27 );
     }
-    
+
     tty = xenStoreDomainGetConsolePath(conn, domid);
     if (tty) {
         virBufferVSprintf(&buf, "    <console tty='%s'/>\n", tty);
Index: xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.41
diff -u -r1.41 xml.c
--- xml.c	21 Sep 2006 15:24:37 -0000	1.41
+++ xml.c	6 Oct 2006 13:52:03 -0000
@@ -921,6 +921,8 @@
     xmlChar *device = NULL;
     xmlChar *source = NULL;
     xmlChar *target = NULL;
+    xmlChar *drvName = NULL;
+    xmlChar *drvType = NULL;
     int ro = 0;
     int typ = 0;
     int cdrom = 0;
@@ -934,7 +936,7 @@
         xmlFree(type);
     }
     device = xmlGetProp(node, BAD_CAST "device");
-    
+
     cur = node->children;
     while (cur != NULL) {
         if (cur->type == XML_ELEMENT_NODE) {
@@ -948,6 +950,11 @@
             } else if ((target == NULL) &&
                        (xmlStrEqual(cur->name, BAD_CAST "target"))) {
                 target = xmlGetProp(cur, BAD_CAST "dev");
+            } else if ((drvName == NULL) &&
+                       (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
+                drvName = xmlGetProp(cur, BAD_CAST "name");
+                if (drvName && !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);
+    /* Normally disks are in a (device (vbd ...)) block
+       but blktap disks ended up in a differently named
+       (device (tap ....)) block.... */
+    if (drvName && !strcmp((const char *)drvName, "tap")) {
+        virBufferAdd(buf, "(tap ", 5);
+    } else {
+        virBufferAdd(buf, "(vbd ", 5);
+    }
 
     if (hvm) {
         char *tmp = (char *)target;
@@ -1000,19 +1014,32 @@
 
         /* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */
         if (xendConfigVersion == 1)
-            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *) tmp);
+            virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *)tmp);
         else /* But newer does not */
-            virBufferVSprintf(buf, "(dev '%s%s')", (const char *) tmp, cdrom ? ":cdrom" : ":disk");
+            virBufferVSprintf(buf, "(dev '%s%s')", (const char *)tmp, cdrom ? ":cdrom" : ":disk");
     } else
-        virBufferVSprintf(buf, "(dev '%s')", (const char *) target);
+        virBufferVSprintf(buf, "(dev '%s')", (const char *)target);
 
-    if (typ == 0)
-        virBufferVSprintf(buf, "(uname 'file:%s')", source);
-    else if (typ == 1) {
-        if (source[0] == '/')
-            virBufferVSprintf(buf, "(uname 'phy:%s')", source);
-        else
-            virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+    if (drvName) {
+        if (!strcmp((const char *)drvName, "tap")) {
+            virBufferVSprintf(buf, "(uname '%s:%s:%s')",
+                              (const char *)drvName,
+                              (drvType ? (const char *)drvType : "aio"),
+                              (const char *)source);
+        } else {
+            virBufferVSprintf(buf, "(uname '%s:%s')",
+                              (const char *)drvName,
+                              (const char *)source);
+        }
+    } else {
+        if (typ == 0)
+            virBufferVSprintf(buf, "(uname 'file:%s')", source);
+        else if (typ == 1) {
+            if (source[0] == '/')
+                virBufferVSprintf(buf, "(uname 'phy:%s')", source);
+            else
+                virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+        }
     }
     if (ro == 0)
         virBufferVSprintf(buf, "(mode 'w')");
@@ -1021,6 +1048,9 @@
 
     virBufferAdd(buf, ")", 1);
     virBufferAdd(buf, ")", 1);
+    xmlFree(drvType);
+    xmlFree(drvName);
+    xmlFree(device);
     xmlFree(target);
     xmlFree(source);
     return (0);

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]