Based on the discussion we have had so far on xl disk configuration handling, I have made a fair amount of implementation changes. As most of you might have suspected the changes are vast and has ripple effect all over the place due to interface changes. But I think I got the go ahead from the maintainers to make these changes for 4.1 and I also believe we are better off making these changes for 4.1. The plan is to send the patches in following order - 1) Patch 1/3 - Interface changes, parsing changes, frontend/endback specific disk parameters setup and other ripple effects caused by interface changes. Ideally, I would like to keep this patch to just the interface and parsing changes but by doing so I might break the rest of the code from even compiling. So, this change is going to be huge. 2) Patch 2/3 - Disk validation changes 3) Patch 3/3 - "xl Disk Configuration option" documentation Let me know if you would prefer a different order for convenience or for some other reason. I have attached the changes I have made so far for you to get an idea of the level of changes coming. Note that this is NOT a patch submission. Here are the parts of the patch I would like to get reviewed - 1) Interface changes - libxl.idl, libxl.h 2) Parsing changes - xl_cmdimpl.c. I have basically divided each disk configuration information into three chunks - pdev, vdev and attribute and parsed it based on our discussion/documentation. In specific, parse_disk_config, parse_disk_attrib, parse_disk_vdev_info and parse_disk_pdev_info implementation is the core of parsing code and that is what I would like to get reviewed to be sure I am not way off from what you would be willing to accept. What not to review in this patch - Anything other than interface and parsing changes, I would request that you ignore for now. I am sending it only for the following reasons - 1) Since we are very close to 4.1, I would like you to get an idea of all the places this change touches. 2) I would want the patch to build! Other than that, there are parts of the patch outside interface/parsing changes that might be incomplete or even incorrect! I will be sending out follow up emails to better understand certain parts of disk hotplug code and disk frontend/backend parameters setup code and will make changes based on the input I get and the patch as a whole should be ready for review after that. If you would rather I resend just the interface/parsing changes for review in that case, let me know. Thanks. Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jan-31 17:18 UTC
Re: [xen-devel][RFC] xl disk configuration handling
On Sun, 30 Jan 2011, Kamala Narasimhan wrote:> diff -r 52e928af3637 tools/libxl/libxl.h > --- a/tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500 > @@ -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; > > 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). 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.> diff -r 52e928af3637 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 > @@ -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), > - 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> @@ -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"; > @@ -963,7 +957,7 @@ int libxl_device_disk_add(libxl_ctx *ctx > } > flexarray_append(back, "params"); > flexarray_append(back, libxl__sprintf(&gc, "%s:%s", > - libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath)); > + libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path)); > > if (libxl__blktap_enabled(&gc)) > device.backend_kind = DEVICE_TAP;It looks like EMPTY, QCOW and QCOW2 disappeared.> @@ -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)) { > - dev = disk->physpath; > + case DISK_PDEV_TYPE_FILE: > + case DISK_PDEV_TYPE_TAP: > + case DISK_PDEV_TYPE_TAP2: > + if ( disk->impl_type == DISK_IMPL_TYPE_AIO ) { > + if (!libxl__blktap_enabled(&gc)) { > + dev = disk->pdev_path; > + break; > + } > + }I wouldn''t take into account "aio" here.> @@ -203,9 +205,11 @@ libxl_device_disk = Struct("device_disk" > libxl_device_disk = Struct("device_disk", [ > ("backend_domid", uint32), > ("domid", domid), > - ("physpath", string), > - ("phystype", libxl_disk_phystype), > - ("virtpath", string), > + ("pdev_path", string), > + ("vdev_path", string), > + ("pdev_type", libxl_disk_pdev_type), > + ("impl_type", libxl_disk_impl_type), > + ("format", libxl_disk_format), > ("unpluggable", integer), > ("readwrite", integer), > ("is_cdrom", integer),we don''t need impl_type> diff -r 52e928af3637 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl_device.c Sun Jan 30 11:47:22 2011 -0500 > @@ -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"; > - case PHYSTYPE_PHY: return "phy"; > - case PHYSTYPE_EMPTY: return "file"; > + switch (pdev_type) { > + case DISK_PDEV_TYPE_FILE: return "file"; > + case DISK_PDEV_TYPE_PHY: return "phy"; > + case DISK_PDEV_TYPE_TAP: return "tap"; > + case DISK_PDEV_TYPE_TAP2: return "tap2"; > default: return NULL; > } > }I think we need to return the format string here, like "vhd", otherwise tapdisk2 wouldn''t work correctly. This function should take a libxl_disk_format as an argument now.> > -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 > --- a/tools/libxl/libxl_dm.c Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl_dm.c Sun Jan 30 11:47:22 2011 -0500 > @@ -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 > --- a/tools/libxl/libxl_internal.h Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl_internal.h Sun Jan 30 11:47:22 2011 -0500 > @@ -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 > --- a/tools/libxl/libxl_utils.c Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/libxl_utils.c Sun Jan 30 11:47:22 2011 -0500 > @@ -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) ) > + *pdev_type = DISK_PDEV_TYPE_PHY; > + else if ( !strncmp(s, "file", sizeof("file")-1) ) > + *pdev_type = DISK_PDEV_TYPE_FILE; > + else if ( !strncmp(s, "tap", sizeof("tap")-1) ) > + *pdev_type = DISK_PDEV_TYPE_TAP; > + else if ( !strncmp(s, "tap2", sizeof("tap2")-1) ) > + *pdev_type = DISK_PDEV_TYPE_TAP2; > + > + return 0; > }Drop tap2.> diff -r 52e928af3637 tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c Fri Jan 28 19:37:49 2011 +0000 > +++ b/tools/libxl/xl_cmdimpl.c Sun Jan 30 11:47:22 2011 -0500 > @@ -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 ) > + return ret_val; > + > + ret_val = parse_disk_vdev_info(disk, buf2); > + if ( ret_val ) > + return ret_val; > + > + return parse_disk_pdev_info(disk, buf2); > } > > static void parse_config_data(const char *configfile_filename_report,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...> @@ -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")) { > - disk.phystype = PHYSTYPE_PHY; > + disk.pdev_type = DISK_PDEV_TYPE_PHY; > } else if (!strcmp(tok, "file")) { > - disk.phystype = PHYSTYPE_FILE; > + disk.pdev_type = DISK_PDEV_TYPE_FILE; > } else if (!strcmp(tok, "tap")) { > tok = strtok(NULL, ":"); > if (!strcmp(tok, "aio")) { > - disk.phystype = PHYSTYPE_AIO; > + disk.impl_type = DISK_IMPL_TYPE_AIO;I would completely ignore "aio:" here. I would also ignore "ioemu:" the same way. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote:> On Sun, 30 Jan 2011, Kamala Narasimhan wrote: > > diff -r 52e928af3637 tools/libxl/libxl.h > > --- a/tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000 > > +++ b/tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500 > > @@ -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; > > > > 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).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?> > diff -r 52e928af3637 tools/libxl/libxl.c > > --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 > > +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 > > @@ -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), > > - 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 readAren''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. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-31 20:19 UTC
Re: [xen-devel][RFC] xl disk configuration handling
> 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).I will switch to two enums instead of three.> 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.Ok.> 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. >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.> 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?>Stefano - I did go through your comments on a subset of code here but as I mentioned in my earlier email, please ignore that code for now as I was going to modify it anyway. It was mostly to help understand the places that require change plus for the code to compile.> > 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).> > I would completely ignore "aio:" here. > I would also ignore "ioemu:" the same way. >This redundant logic in block attach for parsing will be gone and disk parsing logic will be reused. Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-31 20:39 UTC
Re: [xen-devel][RFC] xl disk configuration handling
Ian Campbell wrote:> On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote: >> 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? >Yes, I will go with libxl_disk_backend_type.>>> diff -r 52e928af3637 tools/libxl/libxl.c >>> --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000 >>> +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500 >>> @@ -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), >>> - 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. >I did consider keeping away from such renaming but then I figured I might as well if I am going to make other changes to the interface.> 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.That makes sense. I will rename it to just vdev. Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Feb-01 11:13 UTC
Re: [xen-devel][RFC] xl disk configuration handling
On Mon, 31 Jan 2011, Kamala Narasimhan wrote:> 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.It probably doesn''t, but it is better to limit the scope of the fix at the point.> > 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? >Yep, that would be fine.> > 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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Feb-01 14:00 UTC
Re: [xen-devel][RFC] xl disk configuration handling
>>> 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?Sure, I will go with appropriate DSTATE_* for each chunk we parse. Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel