[xen-devel][PATCH 3/5] xl disk configuration parsing changes

[Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>]

Attached is a patch to refactor xl disk configuration option parsing.  Please 
let me know if there are further comments/issues to fix.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@xxxxxxxxxx>

Kamala

diff -u -r libxl-interface-changes/xl_cmdimpl.c libxl/xl_cmdimpl.c
--- libxl-interface-changes/xl_cmdimpl.c        2011-02-07 12:03:37.000000000 
-0500
+++ libxl/xl_cmdimpl.c  2011-02-07 14:58:39.000000000 -0500
@@ -438,143 +438,184 @@
     return 0;
 }
 
-#define DSTATE_INITIAL   0
-#define DSTATE_TAP       1
-#define DSTATE_PHYSPATH  2
-#define DSTATE_VIRTPATH  3
-#define DSTATE_VIRTTYPE  4
-#define DSTATE_RW        5
-#define DSTATE_TERMINAL  6
+#define DSTATE_INITIAL           0
+#define DSTATE_ATTRIB_PARSED     1
+#define DSTATE_VDEV_PARSED       2
 
+static int parse_disk_attrib(libxl_device_disk *disk, char *attrib)
+{
+    if ( attrib[0] == 'w' )
+        disk->readwrite = 1;
+    else if ( attrib[0] != 'r' ) {
+        LOG("Required access rights attribute is missing or incorrect in "
+            "disk configuration option %s", attrib);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
+static int parse_disk_vdev_info(libxl_device_disk *disk, char *vdev_info)
+{
+    char *vdev, *type;
+
+    type = strchr(vdev_info, ':');
+    if ( type ) {
+        *type = '\0';
+        type++;
+        if ( !strncmp(type, "cdrom", sizeof("cdrom")-1) ) {
+            disk->is_cdrom = 1;
+            disk->unpluggable = 1;
+        }
+        else {
+            LOG("Invalid vdev type %s specified in disk configuration option",
+                type);
+            return ERROR_INVAL;
+        }
+    }
+
+    vdev = vdev_info;
+    if ( vdev[0] == '\0' ) {
+        LOG("Mandatory attribute vdev not specified");
+        return ERROR_INVAL;
+    }
+
+    disk->vdev = strdup(vdev);
+    return 0;
+}
+
+static int parse_disk_pdev_info(libxl_device_disk *disk, char *pdev_info)
+{
+    char *pdev_type, *pdev_impl, *pdev_format, *pdev_path;
+
+    if ( pdev_info[0] == '\0' && !disk->is_cdrom ) {
+        /* pdev can be empty string only for cdrom drive with no media 
inserted */
+        LOG("Required vdev info missing in disk configuration option");
+        return ERROR_INVAL;
+    }
+
+    pdev_path = strrchr(pdev_info, ':');
+    if ( !pdev_path ) {
+        disk->pdev_path = strdup(pdev_info);
+        return 0;
+    }
+
+    *pdev_path = '\0';
+    disk->pdev_path = strdup(++pdev_path);
+
+    pdev_format = strrchr(pdev_info, ':');
+    if ( !pdev_format )
+        pdev_format = pdev_info;
+    else {
+        *pdev_format = '\0';
+        pdev_format++;
+    }
+
+    if ( !strncmp(pdev_format, "raw", sizeof("raw")-1) )
+        disk->format = DISK_FORMAT_RAW;
+    else if ( !strncmp(pdev_format, "qcow", sizeof("qcow")-1) )
+        disk->format = DISK_FORMAT_QCOW;
+    else if ( !strncmp(pdev_format, "qcow2", sizeof("qcow2")-1) )
+        disk->format = DISK_FORMAT_QCOW2;
+    else if ( !strncmp(pdev_format, "vhd", sizeof("vhd")-1) )
+        disk->format = DISK_FORMAT_VHD;
+
+    if ( disk->format == DISK_FORMAT_UNKNOWN )
+        pdev_impl = pdev_format;
+    else {
+        pdev_impl = strrchr(pdev_info, ':');
+        if ( !pdev_impl )
+            return 0;
+        *pdev_impl = '\0';
+        pdev_impl++;
+    }
+
+    if ( !strncmp(pdev_impl, "aio", sizeof("aio")-1) ||
+         !strncmp(pdev_impl, "tapdisk", sizeof("tapdisk")-1) )
+        disk->backend = DISK_BACKEND_TAPDISK2;
+    else if ( !strncmp(pdev_impl, "ioemu", sizeof("ioemu")-1) )
+        disk->backend = DISK_BACKEND_QEMU;
+
+    if ( disk->backend == DISK_BACKEND_UNKNOWN )
+        pdev_type = pdev_impl;
+    else {
+        pdev_type = pdev_info;
+        if ( pdev_type[0] == '\0' )
+            return 0;
+    }
+
+    if ( !strncmp(pdev_type, "phy", sizeof("phy")-1) )
+        disk->backend = DISK_BACKEND_BLKBACK;
+    else if ( !strncmp(pdev_type, "file", sizeof("file")-1) ||
+              !strncmp(pdev_type, "tap", sizeof("tap")-1) ||
+              !strncmp(pdev_type, "tap2", sizeof("tap2")-1) )
+        disk->backend = DISK_BACKEND_TAPDISK2;
+
+    return 0;
+}
+
+/*
+ * Note:  Following code calls methods each of which will parse
+ *        specific chunks of the disk configuration option, the specific
+ *        chunks being attribute, vdev and pdev. As with any parsing code
+ *        it makes assumption about the order in which specific chunks appear
+ *        in the disk configuration option string.  So, please use
+ *        care while modifying the code below, esp. when it comes to
+ *        the order of calls.
+ */
 static int parse_disk_config(libxl_device_disk *disk, char *buf2)
 {
-    int state = DSTATE_INITIAL;
-    char *p, *end, *tok;
+    int state = DSTATE_INITIAL, ret_val;
+    char *substr = buf2;
 
     memset(disk, 0, sizeof(*disk));
 
-    for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
-        switch(state){
-        case DSTATE_INITIAL:
-            if ( *p == ':' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "phy") ) {
-                    state = DSTATE_PHYSPATH;
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_BLKBACK;
-                }else if ( !strcmp(tok, "file") ) {
-                    state = DSTATE_PHYSPATH;
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_TAPDISK2;
-                }else if ( !strcmp(tok, "tap") ) {
-                    state = DSTATE_TAP;
-                }else{
-                    fprintf(stderr, "Unknown disk type: %s\n", tok);
-                    return 0;
+    for ( substr = strrchr(buf2, ',');
+          substr || (!substr && state == DSTATE_VDEV_PARSED);
+          substr = strrchr(buf2, ',') ) {
+        switch ( state ) {
+            case DSTATE_INITIAL: {
+                if ( !substr ) {
+                    LOG("Invalid disk configuration option %s.  Refer to the 
xl disk "
+                    "configuration document for further information", buf2);
+                    return ERROR_INVAL;
                 }
-                tok = p + 1;
-            } else if (*p == ',') {
-                state = DSTATE_VIRTPATH;
-                disk->backend = DISK_FORMAT_EMPTY;
-                disk->pdev_path = strdup("");
-                tok = p + 1;
-            }
-            break;
-        case DSTATE_TAP:
-            if ( *p == ':' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "aio") ) {
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_TAPDISK2;
-                }else if ( !strcmp(tok, "vhd") ) {
-                    disk->format = DISK_FORMAT_VHD;
-                    disk->backend = DISK_BACKEND_TAPDISK2;
-                }else if ( !strcmp(tok, "qcow") ) {
-                    disk->format = DISK_FORMAT_QCOW;
-                    disk->backend = DISK_BACKEND_QEMU;
-                }else if ( !strcmp(tok, "qcow2") ) {
-                    disk->format = DISK_FORMAT_QCOW2;
-                    disk->backend = DISK_BACKEND_QEMU;
-                }else {
-                    fprintf(stderr, "Unknown tapdisk type: %s\n", tok);
-                    return 0;
-                }
-
-                tok = p + 1;
-                state = DSTATE_PHYSPATH;
-            }
-            break;
-        case DSTATE_PHYSPATH:
-            if ( *p == ',' ) {
-                int ioemu_len;
-
-                *p = '\0';
-                disk->pdev_path = (*tok) ? strdup(tok) : NULL;
-                tok = p + 1;
-
-                /* hack for ioemu disk spec */
-                ioemu_len = strlen("ioemu:");
-                state = DSTATE_VIRTPATH;
-                if ( tok + ioemu_len < end &&
-                    !strncmp(tok, "ioemu:", ioemu_len)) {
-                    tok += ioemu_len;
-                    p += ioemu_len;
-                }
-            }
-            break;
-        case DSTATE_VIRTPATH:
-            if ( *p == ',' || *p == ':' || *p == '\0' ) {
-                switch(*p) {
-                case ':':
-                    state = DSTATE_VIRTTYPE;
-                    break;
-                case ',':
-                    state = DSTATE_RW;
-                    break;
-                case '\0':
-                    state = DSTATE_TERMINAL;
-                    break;
-                }
-                if ( tok == p )
-                    goto out;
-                *p = '\0';
-                disk->vdev = (*tok) ? strdup(tok) : NULL;
-                tok = p + 1;
+                *substr = '\0';
+                substr++;
+                ret_val = parse_disk_attrib(disk, substr);
+                if ( ret_val )
+                    return ret_val;
+                state = DSTATE_ATTRIB_PARSED;
+                break;
             }
-            break;
-        case DSTATE_VIRTTYPE:
-            if ( *p == ',' || *p == '\0' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "cdrom") ) {
-                    disk->is_cdrom = 1;
-                    disk->unpluggable = 1;
-                }else{
-                    fprintf(stderr, "Unknown virtual disk type: %s\n", tok);
-                    return 0;
+            case DSTATE_ATTRIB_PARSED: {
+                if ( !substr ) {
+                    LOG("Required vdev info missing in disk configuration 
option");
+                    return ERROR_INVAL;
                 }
-                tok = p + 1;
-                state = (*p == ',') ? DSTATE_RW : DSTATE_TERMINAL;
+                *substr = '\0';
+                substr++;
+                ret_val = parse_disk_vdev_info(disk, substr);
+                if ( ret_val )
+                    return ret_val;
+                state = DSTATE_VDEV_PARSED;
+                break;
             }
-            break;
-        case DSTATE_RW:
-            if ( *p == '\0' ) {
-                disk->readwrite = (tok[0] == 'w');
-                tok = p + 1;
-                state = DSTATE_TERMINAL;
+            case DSTATE_VDEV_PARSED: {
+                ret_val = parse_disk_pdev_info(disk, buf2);
+                if ( ret_val )
+                    return ret_val;
+                if ( disk->format == DISK_FORMAT_UNKNOWN ) 
+                    disk->format = DISK_FORMAT_RAW;
+                if ( disk->backend == DISK_BACKEND_UNKNOWN )
+                    disk->backend = DISK_BACKEND_TAPDISK2;  
+                return 0;
             }
-            break;
-        case DSTATE_TERMINAL:
-            goto out;
         }
     }
 
-out:
-    if ( tok != p || state != DSTATE_TERMINAL ) {
-        fprintf(stderr, "parse error in disk config near '%s'\n", tok);
-        return 0;
-    }
-
-    return 1;
+    LOG("Disk configuration parsing failed");
+    return ERROR_INVAL;
 }
 
 static void parse_config_data(const char *configfile_filename_report,
@@ -768,7 +809,7 @@
 
             d_config->disks = (libxl_device_disk *) realloc(d_config->disks, 
sizeof (libxl_device_disk) * (d_config->num_disks + 1));
             disk = d_config->disks + d_config->num_disks;
-            if ( !parse_disk_config(disk, buf2) ) {
+            if ( parse_disk_config(disk, buf2) ) {
                 exit(1);
             }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel