xl disk configuration handling

diff -r 52e928af3637 tools/libxl/libxl.h

tools/libxl/libxl.h +14 -8

@@ -172,14 +172,27 @@ typedef enum {
 } libxl_console_consback;
 
 typedef enum {
-    PHYSTYPE_QCOW = 1,
-    PHYSTYPE_QCOW2,
-    PHYSTYPE_VHD,
-    PHYSTYPE_AIO,
-    PHYSTYPE_FILE,
-    PHYSTYPE_PHY,
-    PHYSTYPE_EMPTY,
-} libxl_disk_phystype;
+    DISK_FORMAT_UNKNOWN = 0,
+    DISK_FORMAT_QCOW,
+    DISK_FORMAT_QCOW2,
+    DISK_FORMAT_VHD,
+    DISK_FORMAT_RAW,
+} libxl_disk_format;
+
+typedef enum {
+    DISK_IMPL_TYPE_UNKNOWN = 0,
+    DISK_IMPL_TYPE_AIO,
+    DISK_IMPL_TYPE_TAPDISK,
+    DISK_IMPL_TYPE_IOEMU,
+} libxl_disk_impl_type;
+

tools/libxl/libxl.c +9 -15

@@ -920,37 +920,31 @@ int libxl_device_disk_add(libxl_ctx *ctx
     device.domid = disk->domid;
     device.kind = DEVICE_VBD;
 
-    switch (disk->phystype) {
-        case PHYSTYPE_PHY: {
+    switch (disk->pdev_type) {
+        case DISK_PDEV_TYPE_PHY: {
 
-            libxl__device_physdisk_major_minor(disk->physpath, &major, &minor);
+            libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
             flexarray_append(back, "physical-device");
             flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
 
             flexarray_append(back, "params");
-            flexarray_append(back, disk->physpath);
+            flexarray_append(back, disk->pdev_path);
 
             device.backend_kind = DEVICE_VBD;
             break;
         }
-        case PHYSTYPE_EMPTY:
-            break;
-        case PHYSTYPE_FILE:
-            /* let's pretend is tap:aio for the moment */
-            disk->phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO:
-        case PHYSTYPE_QCOW:
-        case PHYSTYPE_QCOW2:
-        case PHYSTYPE_VHD:
+        case DISK_PDEV_TYPE_FILE:
+        case DISK_PDEV_TYPE_TAP:
+        case DISK_PDEV_TYPE_TAP2:
             if (libxl__blktap_enabled(&gc)) {
                 const char *dev = libxl__blktap_devpath(&gc,
-                                               disk->physpath, disk->phystype);
+                                               disk->pdev_path, disk->pdev_type);
                 if (!dev) {
                     rc = ERROR_FAIL;
                     goto out_free;
                 }
                 flexarray_append(back, "tapdisk-params");
-                flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
+                flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path));
                 flexarray_append(back, "params");
                 flexarray_append(back, libxl__strdup(&gc, dev));
                 backend_type = "phy";

tools/libxl/libxl.c +4 -9

@@ -1044,32 +1038,33 @@ char * libxl_device_disk_local_attach(li
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     const char *dev = NULL;
     char *ret = NULL;
-    int phystype = disk->phystype;
+    int phystype = disk->pdev_type;
     switch (phystype) {
-        case PHYSTYPE_PHY: {
-            fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->physpath);
-            dev = disk->physpath;
+        case DISK_PDEV_TYPE_PHY: {
+            fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->pdev_path);
+            dev = disk->pdev_path;
             break;
         }
-        case PHYSTYPE_FILE:
-            /* let's pretend is tap:aio for the moment */
-            phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO:
-            if (!libxl__blktap_enabled(&gc)) {

tools/libxl/libxl.c +0 -1

@@ -203,9 +205,11 @@ libxl_device_disk = Struct("device_disk"
 libxl_device_disk = Struct("device_disk", [
     ("backend_domid", uint32),
     ("domid", domid),
-    ("physpath", string),

diff -r 52e928af3637 tools/libxl/libxl_device.c

tools/libxl/libxl_device.c +1 -7

@@ -121,31 +121,23 @@ out:
     return rc;
 }
 
-char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype)
+char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type)
 {
-    switch (phystype) {
-        case PHYSTYPE_QCOW: return "qcow";
-        case PHYSTYPE_QCOW2: return "qcow2";
-        case PHYSTYPE_VHD: return "vhd";
-        case PHYSTYPE_AIO: return "aio";
-        case PHYSTYPE_FILE: return "file";

tools/libxl/libxl_device.c +4 -9

-char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype)
+char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type)
 {
-    switch (phystype) {
-        case PHYSTYPE_QCOW: return "tap";
-        case PHYSTYPE_QCOW2: return "tap";
-        case PHYSTYPE_VHD: return "tap";
-        case PHYSTYPE_AIO: return "tap";
+    switch (pdev_type) {
         /* let's pretend file is tap:aio */
-        case PHYSTYPE_FILE: return "tap";
-        case PHYSTYPE_EMPTY: return "tap";
-        case PHYSTYPE_PHY: return "phy";
+        case DISK_PDEV_TYPE_FILE: return "tap";
+        case DISK_PDEV_TYPE_PHY: return "phy";
         default: return NULL;
     }
 }

diff -r 52e928af3637 tools/libxl/libxl_dm.c

tools/libxl/libxl_dm.c +3 -3

@@ -303,10 +303,10 @@ static char ** libxl_build_device_model_
         for (i; i < nb; i++) {
             if (disks[i].is_cdrom) {
                 flexarray_append(dm_args, "-cdrom");
-                flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
+                flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
             } else {
-                flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].virtpath));
-                flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
+                flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].vdev_path));
+                flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
             }
             libxl_device_disk_destroy(&disks[i]);
         }

diff -r 52e928af3637 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +3 -3

@@ -178,8 +178,8 @@ _hidden void libxl__userdata_destroyall(
 _hidden void libxl__userdata_destroyall(libxl_ctx *ctx, uint32_t domid);
 
 /* from xl_device */
-_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
-_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype);
+_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type);
+_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type);
 
 _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 _hidden int libxl__device_disk_dev_number(char *virtpath);

@@ -306,7 +306,7 @@ _hidden int libxl__blktap_enabled(libxl_
  */
 _hidden const char *libxl__blktap_devpath(libxl__gc *gc,
                                  const char *disk,
-                                 libxl_disk_phystype phystype);
+                                 libxl_disk_pdev_type pdev_type);
 
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 

diff -r 52e928af3637 tools/libxl/libxl_utils.c

tools/libxl/libxl_utils.c +3 -26

@@ -275,34 +275,20 @@ out:
     return rc;
 }
 
-int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_phystype *phystype)
+int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_pdev_type *pdev_type)
 {
-    char *p;
-    int rc = 0;
+    *pdev_type = DISK_PDEV_TYPE_UNKNOWN;
 
-    if (!strcmp(s, "phy")) {
-        *phystype = PHYSTYPE_PHY;
-    } else if (!strcmp(s, "file")) {
-        *phystype = PHYSTYPE_FILE;
-    } else if (!strcmp(s, "tap")) {
-        p = strchr(s, ':');
-        if (!p) {
-            rc = ERROR_INVAL;
-            goto out;
-        }
-        p++;
-        if (!strcmp(p, "aio")) {
-            *phystype = PHYSTYPE_AIO;
-        } else if (!strcmp(p, "vhd")) {
-            *phystype = PHYSTYPE_VHD;
-        } else if (!strcmp(p, "qcow")) {
-            *phystype = PHYSTYPE_QCOW;
-        } else if (!strcmp(p, "qcow2")) {
-            *phystype = PHYSTYPE_QCOW2;
-        }
-    }
-out:
-    return rc;
+    if ( !strncmp(s, "phy", sizeof("phy")-1) )

diff -r 52e928af3637 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +149 -129

@@ -361,9 +361,9 @@ static void printf_info(int domid,
         printf("\t\t(tap\n");
         printf("\t\t\t(backend_domid %d)\n", d_config->disks[i].backend_domid);
         printf("\t\t\t(domid %d)\n", d_config->disks[i].domid);
-        printf("\t\t\t(physpath %s)\n", d_config->disks[i].physpath);
-        printf("\t\t\t(phystype %d)\n", d_config->disks[i].phystype);
-        printf("\t\t\t(virtpath %s)\n", d_config->disks[i].virtpath);
+        printf("\t\t\t(physpath %s)\n", d_config->disks[i].pdev_path);
+        printf("\t\t\t(phystype %d)\n", d_config->disks[i].pdev_type);
+        printf("\t\t\t(virtpath %s)\n", d_config->disks[i].vdev_path);
         printf("\t\t\t(unpluggable %d)\n", d_config->disks[i].unpluggable);
         printf("\t\t\t(readwrite %d)\n", d_config->disks[i].readwrite);
         printf("\t\t\t(is_cdrom %d)\n", d_config->disks[i].is_cdrom);

@@ -438,137 +438,164 @@ static int parse_action_on_shutdown(cons
     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
-
+static int parse_disk_attrib(libxl_device_disk *disk, char *buf2)
+{
+    char *attrib = strrchr(buf2, ',');
+    if ( !attrib ) {
+        LOG("Invalid disk configuration option %s.  Refer to the xl disk "
+            "configuration document for further information", buf2);
+        return ERROR_INVAL;
+    }
+
+    *attrib = '\0';
+    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", buf2);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
+static int parse_disk_vdev_info(libxl_device_disk *disk, char *buf2)
+{
+    char *vdev_info, *vdev, *type;
+
+    vdev_info = strrchr(buf2, ',');
+    if ( !vdev_info ) {
+        LOG("Required vdev info missing in disk configuration option");
+        return ERROR_INVAL;
+    }
+
+    *vdev_info = '\0';
+    vdev_info++;
+
+    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_path = strdup(vdev);
+    return 0;    
+}
+
+static int parse_disk_pdev_info(libxl_device_disk *disk, char *buf2)
+{
+    char *pdev_info, *pdev_type, *pdev_impl, *pdev_format, *pdev_path;
+
+    pdev_info = buf2;
+    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) )
+        disk->impl_type = DISK_IMPL_TYPE_AIO;
+    else if ( !strncmp(pdev_impl, "tapdisk", sizeof("tapdisk")-1) )
+        disk->impl_type = DISK_IMPL_TYPE_TAPDISK;
+    else if ( !strncmp(pdev_impl, "ioemu", sizeof("ioemu")-1) )
+        disk->impl_type = DISK_IMPL_TYPE_IOEMU;
+
+    if ( disk->impl_type == DISK_IMPL_TYPE_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->pdev_type = DISK_PDEV_TYPE_PHY;
+    else if ( !strncmp(pdev_type, "file", sizeof("file")-1) )
+        disk->pdev_type = DISK_PDEV_TYPE_FILE;
+    else if ( !strncmp(pdev_type, "tap", sizeof("tap")-1) )
+        disk->pdev_type = DISK_PDEV_TYPE_TAP;
+    else if ( !strncmp(pdev_type, "tap2", sizeof("tap2")-1) )
+        disk->pdev_type = DISK_PDEV_TYPE_TAP2;
+
+    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 ret_val;
 
     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->phystype = PHYSTYPE_PHY;
-                }else if ( !strcmp(tok, "file") ) {
-                    state = DSTATE_PHYSPATH;
-                    disk->phystype = PHYSTYPE_FILE;
-                }else if ( !strcmp(tok, "tap") ) {
-                    state = DSTATE_TAP;
-                }else{
-                    fprintf(stderr, "Unknown disk type: %s\n", tok);
-                    return 0;
-                }
-                tok = p + 1;
-            } else if (*p == ',') {
-                state = DSTATE_VIRTPATH;
-                disk->phystype = PHYSTYPE_EMPTY;
-                disk->physpath = strdup("");
-                tok = p + 1;
-            }
-            break;
-        case DSTATE_TAP:
-            if ( *p == ':' ) {
-                *p = '\0';
-                if ( !strcmp(tok, "aio") ) {
-                    disk->phystype = PHYSTYPE_AIO;
-                }else if ( !strcmp(tok, "vhd") ) {
-                    disk->phystype = PHYSTYPE_VHD;
-                }else if ( !strcmp(tok, "qcow") ) {
-                    disk->phystype = PHYSTYPE_QCOW;
-                }else if ( !strcmp(tok, "qcow2") ) {
-                    disk->phystype = PHYSTYPE_QCOW2;
-                }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->physpath = (*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->virtpath = (*tok) ? strdup(tok) : NULL;
-                tok = p + 1;
-            }
-            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;
-                }
-                tok = p + 1;
-                state = (*p == ',') ? DSTATE_RW : DSTATE_TERMINAL;
-            }
-            break;
-        case DSTATE_RW:
-            if ( *p == '\0' ) {
-                disk->readwrite = (tok[0] == 'w');
-                tok = p + 1;
-                state = DSTATE_TERMINAL;
-            }
-            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;
+    ret_val = parse_disk_attrib(disk, buf2);
+    if ( ret_val )

tools/libxl/xl_cmdimpl.c +11 -12

@@ -762,7 +789,7 @@ static void parse_config_data(const char
 
             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);
             }
 

@@ -1838,25 +1865,24 @@ static void cd_insert(const char *dom, c
         p = strchr(phys, ':');
         if (!p) {
             fprintf(stderr, "No type specified, ");
-            disk.physpath = phys;
+            disk.pdev_path = phys;
             if (!strncmp(phys, "/dev", 4)) {
                 fprintf(stderr, "assuming phy:\n");
-                disk.phystype = PHYSTYPE_PHY;
+                disk.pdev_type = DISK_PDEV_TYPE_PHY;
             } else {
                 fprintf(stderr, "assuming file:\n");
-                disk.phystype = PHYSTYPE_FILE;
+                disk.pdev_type = DISK_PDEV_TYPE_FILE;
             }
         } else {
             *p = '\0';
             p++;
-            disk.physpath = p;
-            libxl_string_to_phystype(&ctx, phys, &disk.phystype);
-        }
-    } else {
-            disk.physpath = strdup("");
-            disk.phystype = PHYSTYPE_EMPTY;
-    }
-    disk.virtpath = (char*)virtdev;
+            disk.pdev_path = p;
+            libxl_string_to_phystype(&ctx, phys, &disk.pdev_type);
+        }
+    } else 
+        disk.pdev_path = strdup("");
+
+    disk.vdev_path = (char*)virtdev;
     disk.unpluggable = 1;
     disk.readwrite = 0;
     disk.is_cdrom = 1;

@@ -4375,19 +4401,19 @@ int main_blockattach(int argc, char **ar
 
     tok = strtok(argv[optind+1], ":");
     if (!strcmp(tok, "phy")) {

Quoted 37 lines · Kamala Narasimhan

diff -r 52e928af3637 tools/libxl/libxl.h

tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000 +21 -8

@@ -172,14 +172,27 @@ typedef enum {
 } libxl_console_consback;
 
 typedef enum {
-    PHYSTYPE_QCOW = 1,
-    PHYSTYPE_QCOW2,
-    PHYSTYPE_VHD,
-    PHYSTYPE_AIO,
-    PHYSTYPE_FILE,
-    PHYSTYPE_PHY,
-    PHYSTYPE_EMPTY,
-} libxl_disk_phystype;
+    DISK_FORMAT_UNKNOWN = 0,
+    DISK_FORMAT_QCOW,
+    DISK_FORMAT_QCOW2,
+    DISK_FORMAT_VHD,
+    DISK_FORMAT_RAW,
+} libxl_disk_format;
+
+typedef enum {
+    DISK_IMPL_TYPE_UNKNOWN = 0,
+    DISK_IMPL_TYPE_AIO,
+    DISK_IMPL_TYPE_TAPDISK,
+    DISK_IMPL_TYPE_IOEMU,
+} libxl_disk_impl_type;
+
+typedef enum {
+    DISK_PDEV_TYPE_UNKNOWN = 0,
+    DISK_PDEV_TYPE_PHY,
+    DISK_PDEV_TYPE_FILE,
+    DISK_PDEV_TYPE_TAP,
+    DISK_PDEV_TYPE_TAP2,
+} libxl_disk_pdev_type;
 

tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500 typedef enum { · line 198

Quoted 2 lines

tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000 +0 -0

 typedef enum {
     NICTYPE_IOEMU = 1,

I agree that libxl_disk_phystype expresses both the format and the backend type in a single confusing way, so there should be two enums:
one for the format (libxl_disk_format) and one for the backend type (libxl_disk_pdev_type).

Quoted 7 lines

diff -r 52e928af3637 tools/libxl/libxl.c

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 +0 -0

@@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
     for (i = 0; i < num_disks; i++) {
         if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
                      libxl__xs_get_dompath(&gc, domid),

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 int libxl_wait_for_disk_ejects(libxl_ctx · line 594

Quoted 5 lines

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 +1 -1

-                     libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
+                     libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
             goto out;
         if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
             goto out;

it would be nice if all the renaming was done in a separate patch so that the real changes are easier to read

Quoted 4 lines · Stefano Stabellini

I agree that libxl_disk_phystype expresses both the format and the backend type in a single confusing way, so there should be two enums:
one for the format (libxl_disk_format) and one for the backend type (libxl_disk_pdev_type).

pdev_type doesn't seem like a very good name for "backend type" (and it is unnecessarily abbreviated which I personally dislike, especially in public interfaces).

Would libxl_disk_backend_type work?

Quoted 6 lines

Quoted 6 lines

diff -r 52e928af3637 tools/libxl/libxl.c

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 +0 -0

@@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
     for (i = 0; i < num_disks; i++) {
         if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",

Quoted 8 lines

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 int libxl_wait_for_disk_ejects(libxl_ctx · line 594

Quoted 6 lines

tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 renamed from tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 +1 -1

                      libxl__xs_get_dompath(&gc, domid),
-                     libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
+                     libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
             goto out;
         if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
             goto out;

it would be nice if all the renaming was done in a separate patch so that the real changes are easier to read

Aren't the renamings a bit cosmetic anyway, i.e. could/should be left for 4.2? I guess I can see the argument of changing the field name if the type name changes, assuming the type names are well chosen.

The virtpath->vdev_path rename in particular seems odd. The main thing which is wrong with virtpath is that the thing it contains is a vdev which doesn't really have any path-like properties, but we retain the path part in the new name. If it's renamed to anything I reckon it should just be vdev.

I agree that libxl_disk_phystype expresses both the format and the backend type in a single confusing way, so there should be two enums:
one for the format (libxl_disk_format) and one for the backend type (libxl_disk_pdev_type).

However I don't think libxl_disk_impl_type should be exposed at all, it should be up to libxl to decide whether AIO should be enabled or not. It might be useful to let the user disable the PV interface for a particular disk (that is what ioemu stands for), but it is too late for 4.1, let's just ignore ioemu for the moment.

The backend types should be BLKBACK, TAPDISK2, and QEMU; TAPDISK refers to blktap1 that is not supported by libxl. However libxl uses "tap:" as backend string corresponding to TAPDISK2, I understand that might be confusing but I wouldn't change it now.
Also it might be useful to retain the EMPTY format among the various libxl_disk_format's, it should reduce the overall amount of changes.

it would be nice if all the renaming was done in a separate patch so that the real changes are easier to read

do we really need to change the parsing function that much? I understand there are significant changes but this is a total rewrite. I am concerned about all the bugs we might find later after the release...

I would completely ignore "aio:" here.
I would also ignore "ioemu:" the same way.

EMPTY, an indicator that there is no media in the cd-rom drive didn't really go with the any of the enums which is why I removed it. But later when I was changing code I did find it inconvenient to check for both empty path plus cdrom, so I will add it to disk format types though I am really not sure if it belongs there.

Quoted 3 lines

it would be nice if all the renaming was done in a separate patch so that the real changes are easier to read

I was worried you may not accept a patch with just renaming changes! I could separate interface changes (which would include renaming) from parsing and send them as two separate patches. Would that be ok?

Quoted 5 lines

do we really need to change the parsing function that much? I understand there are significant changes but this is a total rewrite. I am concerned about all the bugs we might find later after the release...

This is one change I would really like to go with. Not only does it help with the changes we needed, it also gets rid of code duplication. With this change block-attach can rely on the same parsing code (that is once I submit the block-attach changes patch).

Quoted 9 lines

Quoted 5 lines

do we really need to change the parsing function that much? I understand there are significant changes but this is a total rewrite. I am concerned about all the bugs we might find later after the release...

This is one change I would really like to go with. Not only does it help with the changes we needed, it also gets rid of code duplication. With this change block-attach can rely on the same parsing code (that is once I submit the block-attach changes patch).

It took us several iterations to get the parsing right, I would like to keep the state machine and the field parsing as it is, but each case could have its own function to implement it. In other words, would you be OK with calling parse_disk_attrib, parse_disk_vdev_info and parse_disk_pdev_info from the main switch under DSTATE_PHYSPATH, DSTATE_VIRTPATH, etc?