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

[Libvir] [PATCH] Refactor xend_internal.c block device detection



This patch is a little bit esoteric, but I need it for something I'm working on at the moment.

At the moment the code in xend_internal.c: xend_parse_sexp_desc parses the domain sexpr directly into XML. This makes it rather hard to just get a list of block devices without repeating the same code. So here I've factored out the common code for parsing block devices into a separate function and callback.

There are a couple of small changes that you should be aware of: (1) The <devices> list may be returned in a different order (specifically, block devices are always returned first). (2) We iterate over the root nodes of the sexpr twice (once for block devices, once for vifs and vfbs). But the sexpr is small and in-memory so this shouldn't be a problem compared to having to do the HTTP request to xend to get it in the first place.

'make check' passes.

Rich.

--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903
? src/driver.h.new
? src/libvirt.c.new
? src/xen_unified.c.new
? src/xend_internal.c.new
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.148
diff -u -p -r1.148 xend_internal.c
--- src/xend_internal.c	15 Oct 2007 21:38:56 -0000	1.148
+++ src/xend_internal.c	16 Oct 2007 13:05:28 -0000
@@ -1334,6 +1334,213 @@ xend_parse_sexp_desc_os(virConnectPtr xe
     return(0);
 }
 
+typedef int
+  (*sexp_blockdevs_cb)
+  (virConnectPtr conn, void *data,
+   int isBlock, int cdrom, int isNoSrcCdrom,
+   char *drvName, char *drvType,
+   const char *src, const char *dst,
+   const char *mode);
+
+/**
+ * xend_parse_sexp_blockdevs:
+ * @conn: connection
+ * @root: root sexpr
+ * @xendConfigVersion: version of xend
+ * @fn: callback function
+ * @data: optional data for callback function
+ *
+ * This parses out block devices from the domain sexpr and calls
+ * fn (conn, data, ...) for each block device found.
+ *
+ * Returns 0 if successful or -1 if failed.
+ */
+static int
+xend_parse_sexp_blockdevs (virConnectPtr conn, struct sexpr *root,
+                           int xendConfigVersion,
+                           sexp_blockdevs_cb fn, void *data)
+{
+    struct sexpr *cur, *node;
+    int hvm;
+
+    hvm = sexpr_lookup(root, "domain/image/hvm") ? 1 : 0;
+
+    for (cur = root; cur->kind == SEXPR_CONS; cur = cur->u.s.cdr) {
+        node = cur->u.s.car;
+        /* 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 ret = -1;
+            int isBlock = 0;
+            int cdrom = 0;
+            int isNoSrcCdrom = 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 (dst == NULL) {
+                virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                             _("domain information incomplete, vbd has no dev"));
+                goto bad_parse;
+            }
+
+            if (src == NULL) {
+                /* There is a case without the uname to the CD-ROM device */
+                offset = strchr(dst, ':');
+                if (offset) {
+                    if (hvm && !strcmp( offset , ":cdrom")) {
+                        isNoSrcCdrom = 1;
+                    }
+                    offset[0] = '\0';
+                }
+                if (!isNoSrcCdrom) {
+                    virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                                 _("domain information incomplete, vbd has no src"));
+                    goto bad_parse;
+                }
+            }
+
+            if (!isNoSrcCdrom) {
+                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;
+                    }
+
+                    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;
+
+            /* New style disk config from Xen >= 3.0.3 */
+            if (xendConfigVersion > 1) {
+                offset = strrchr(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';
+                }
+            }
+
+            /* Call the callback function. */
+            ret = fn (conn, data, isBlock, cdrom, isNoSrcCdrom,
+                      drvName, drvType, src, dst, mode);
+
+        bad_parse:
+            if (drvName)
+                free(drvName);
+            if (drvType)
+                free(drvType);
+
+            if (ret == -1) return -1;
+        }
+    }
+
+    return 0;
+}
+
+static int
+xend_parse_sexp_desc_blockdev (virConnectPtr conn ATTRIBUTE_UNUSED,
+                               void *data,
+                               int isBlock, int cdrom, int isNoSrcCdrom,
+                               char *drvName, char *drvType,
+                               const char *src, const char *dst,
+                               const char *mode)
+{
+    virBuffer *buf = (virBuffer *) data;
+
+    if (!isNoSrcCdrom) {
+        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);
+        } else {
+            virBufferVSprintf(buf, "      <source file='%s'/>\n", src);
+        }
+    } else {
+        /* This case is the cdrom device only */
+        virBufferVSprintf(buf, "    <disk device='cdrom'>\n");
+    }
+    virBufferVSprintf(buf, "      <target dev='%s'/>\n", dst);
+
+    /* 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");
+    else if ((mode != NULL) && (!strcmp(mode, "w!")))
+        virBufferVSprintf(buf, "      <shareable/>\n");
+    virBufferAdd(buf, "    </disk>\n", 12);
+
+    return 0;
+}
+
 /**
  * xend_parse_sexp_desc:
  * @conn: the connection associated with the XML
@@ -1470,158 +1677,16 @@ xend_parse_sexp_desc(virConnectPtr conn,
     if ((tmp != NULL) && (tmp[0] != 0))
         virBufferVSprintf(&buf, "    <emulator>%s</emulator>\n", tmp);
 
+    /* append block devices */
+    if (xend_parse_sexp_blockdevs (conn, root, xendConfigVersion,
+                                   xend_parse_sexp_desc_blockdev, &buf) == -1)
+        goto error;
+
+    /* append network devices and framebuffer */
     for (cur = root; cur->kind == SEXPR_CONS; cur = cur->u.s.cdr) {
         node = cur->u.s.car;
-        /* 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;
-            int isNoSrcCdrom = 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 (dst == NULL) {
-                virXendError(conn, VIR_ERR_INTERNAL_ERROR,
-                             _("domain information incomplete, vbd has no dev"));
-                goto bad_parse;
-            }
-
-            if (src == NULL) {
-                /* There is a case without the uname to the CD-ROM device */
-                offset = strchr(dst, ':');
-                if (offset) {
-                    if (hvm && !strcmp( offset , ":cdrom")) {
-                        isNoSrcCdrom = 1;
-                    }
-                    offset[0] = '\0';
-                }
-                if (!isNoSrcCdrom) {
-                    virXendError(conn, VIR_ERR_INTERNAL_ERROR,
-                                 _("domain information incomplete, vbd has no src"));
-                    goto bad_parse;
-                }
-            }
-
-            if (!isNoSrcCdrom) {
-                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;
-                    }
 
-                    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;
-
-            /* New style disk config from Xen >= 3.0.3 */
-            if (xendConfigVersion > 1) {
-                offset = strrchr(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';
-                }
-            }
-
-            if (!isNoSrcCdrom) {
-                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);
-                } else {
-                    virBufferVSprintf(&buf, "      <source file='%s'/>\n", src);
-                }
-            } else {
-                /* This case is the cdrom device only */
-                virBufferVSprintf(&buf, "    <disk device='cdrom'>\n");
-            }
-            virBufferVSprintf(&buf, "      <target dev='%s'/>\n", dst);
-
-
-            /* 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");
-	    else if ((mode != NULL) && (!strcmp(mode, "w!")))
-                virBufferVSprintf(&buf, "      <shareable/>\n");
-            virBufferAdd(&buf, "    </disk>\n", 12);
-
-            bad_parse:
-            if (drvName)
-                free(drvName);
-            if (drvType)
-                free(drvType);
-        } else if (sexpr_lookup(node, "device/vif")) {
+        if (sexpr_lookup(node, "device/vif")) {
             const char *tmp2;
             tmp2 = sexpr_node(node, "device/vif/script");
             tmp = sexpr_node(node, "device/vif/bridge");
Index: tests/sexpr2xmldata/sexpr2xml-curmem.xml
===================================================================
RCS file: /data/cvs/libvirt/tests/sexpr2xmldata/sexpr2xml-curmem.xml,v
retrieving revision 1.4
diff -u -p -r1.4 sexpr2xml-curmem.xml
--- tests/sexpr2xmldata/sexpr2xml-curmem.xml	21 Aug 2007 08:54:07 -0000	1.4
+++ tests/sexpr2xmldata/sexpr2xml-curmem.xml	16 Oct 2007 13:05:28 -0000
@@ -15,17 +15,17 @@
   <on_reboot>restart</on_reboot>
   <on_crash>restart</on_crash>
   <devices>
+    <disk type='file' device='disk'>
+      <driver name='tap' type='aio'/>
+      <source file='/xen/rhel5.img'/>
+      <target dev='xvda:disk'/>
+    </disk>
     <interface type='bridge'>
       <source bridge='xenbr0'/>
       <target dev='vif5.0'/>
       <mac address='00:16:3e:1d:06:15'/>
       <script path='vif-bridge'/>
     </interface>
-    <disk type='file' device='disk'>
-      <driver name='tap' type='aio'/>
-      <source file='/xen/rhel5.img'/>
-      <target dev='xvda:disk'/>
-    </disk>
     <input type='mouse' bus='xen'/>
     <graphics type='vnc' port='5905'/>
   </devices>
Index: tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml
===================================================================
RCS file: /data/cvs/libvirt/tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml,v
retrieving revision 1.8
diff -u -p -r1.8 sexpr2xml-no-source-cdrom.xml
--- tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml	30 Sep 2007 15:36:47 -0000	1.8
+++ tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml	16 Oct 2007 13:05:28 -0000
@@ -20,11 +20,6 @@
   <clock offset='utc'/>
   <devices>
     <emulator>/usr/lib/xen/bin/qemu-dm</emulator>
-    <interface type='bridge'>
-      <source bridge='xenbr0'/>
-      <target dev='vif6.0'/>
-      <mac address='00:16:3e:0a:7b:39'/>
-    </interface>
     <disk type='block' device='disk'>
       <driver name='phy'/>
       <source dev='/dev/sda8'/>
@@ -34,6 +29,11 @@
       <target dev='hdc'/>
       <readonly/>
     </disk>
+    <interface type='bridge'>
+      <source bridge='xenbr0'/>
+      <target dev='vif6.0'/>
+      <mac address='00:16:3e:0a:7b:39'/>
+    </interface>
     <input type='mouse' bus='ps2'/>
     <graphics type='vnc' port='5906'/>
   </devices>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


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