Kamala Narasimhan
2011-Feb-07 21:24 UTC
[xen-devel][PATCH 3/5] xl disk configuration parsing changes
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@citrix.com> Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Feb-08 16:43 UTC
Re: [xen-devel][PATCH 3/5] xl disk configuration parsing changes
On Mon, 7 Feb 2011, Kamala Narasimhan wrote:> 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 2Why did you need to remove 4 states? Also it would be nice to split this patch in two: a patch that introduces parse_disk_attrib, parse_disk_vdev_info, etc, and a second patch that makes any necessary changes to the state machine. Otherwise it is very difficult to understand the reason behind each change to the state machine.> +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; > }There doesn''t seem to be a way to set DISK_FORMAT_EMPTY anymore, in fact a config line like '',hdc:cdrom,w'' seems to be broken. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Feb-08 19:08 UTC
Re: [xen-devel][PATCH 3/5] xl disk configuration parsing changes
>> -#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 > > Why did you need to remove 4 states?I didn''t have a need for it.> Also it would be nice to split this patch in two: a patch that > introduces parse_disk_attrib, parse_disk_vdev_info, etc, and a second > patch that makes any necessary changes to the state machine. > Otherwise it is very difficult to understand the reason behind each > change to the state machine. >That might be a little tricky. I will talk to you on this offline.>> static int parse_disk_config(libxl_device_disk *disk, char *buf2) >> { > > There doesn''t seem to be a way to set DISK_FORMAT_EMPTY anymore, in fact > a config line like '',hdc:cdrom,w'' seems to be broken. >Patch 5/5, validation code should take care of it. Kamala _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel