Gianni Tedesco
2010-Aug-19 14:34 UTC
[Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:
Switch to a state machine parser since it''s easier to handle all these
exotic cases without segfaulting. NULL physpaths are now allowed and a
dodgy hack is introduced to skip over the "ioemu:" prefix for a
virtpath. Also fixes a leak of buf2.
Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
diff -r 6bd04080ab99 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed Aug 18 18:06:10 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c Thu Aug 19 15:34:20 2010 +0100
@@ -539,6 +539,134 @@ 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_config(libxl_device_disk *disk, char *buf2)
+{
+ int state = DSTATE_INITIAL;
+ char *p, *end, *tok;
+
+ 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;
+ }
+ 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;
+}
+
static void parse_config_data(const char *configfile_filename_report,
const char *configfile_data,
int configfile_len,
@@ -716,59 +844,13 @@ static void parse_config_data(const char
while ((buf = xlu_cfg_get_listitem (vbds, d_config->num_disks)) !=
NULL) {
libxl_device_disk *disk;
char *buf2 = strdup(buf);
- char *p, *p2;
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;
-
- disk->backend_domid = 0;
- disk->domid = 0;
- disk->unpluggable = 0;
-
- p = strtok(buf2, ",:");
- while (*p == '' '')
- p++;
- if (!strcmp(p, "phy")) {
- disk->phystype = PHYSTYPE_PHY;
- } else if (!strcmp(p, "file")) {
- disk->phystype = PHYSTYPE_FILE;
- } else if (!strcmp(p, "tap")) {
- p = strtok(NULL, ":");
- if (!strcmp(p, "aio")) {
- disk->phystype = PHYSTYPE_AIO;
- } else if (!strcmp(p, "vhd")) {
- disk->phystype = PHYSTYPE_VHD;
- } else if (!strcmp(p, "qcow")) {
- disk->phystype = PHYSTYPE_QCOW;
- } else if (!strcmp(p, "qcow2")) {
- disk->phystype = PHYSTYPE_QCOW2;
- }
+ if ( !parse_disk_config(disk, buf2) ) {
+ exit(1);
}
- p = strtok(NULL, ",");
- while (*p == '' '')
- p++;
- disk->physpath= strdup(p);
- p = strtok(NULL, ",");
- while (*p == '' '')
- p++;
- p2 = strchr(p, '':'');
- if (p2 == NULL) {
- disk->virtpath = strdup(p);
- disk->is_cdrom = 0;
- disk->unpluggable = 1;
- } else {
- *p2 = ''\0'';
- disk->virtpath = strdup(p);
- if (!strcmp(p2 + 1, "cdrom")) {
- disk->is_cdrom = 1;
- disk->unpluggable = 1;
- } else
- disk->is_cdrom = 0;
- }
- p = strtok(NULL, ",");
- while (*p == '' '')
- p++;
- disk->readwrite = (p[0] == ''w'') ? 1 : 0;
+
free(buf2);
d_config->num_disks++;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Aug-19 14:54 UTC
Re: [Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:
Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don''t segfault
parsing disk configs, support NULL physpath and
ioemu:"):> Switch to a state machine parser since it''s easier to handle all
these
> exotic cases without segfaulting. NULL physpaths are now allowed and a
> dodgy hack is introduced to skip over the "ioemu:" prefix for a
> virtpath. Also fixes a leak of buf2.
I''m not convinced that it is clearer. It''s certainly a lot
longer:
132 lines added for 50 removed.
Perhaps we should just import a regexp parser and use that ? Really,
we should have a regexp or some other kind of declarative statement of
the syntax.
Possible regexp parsers include pcre and flex. flex has the advantage
that we''re using it already so it''s just another line in the
Makefile.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Aug-19 14:58 UTC
Re: [Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:
On Thu, 2010-08-19 at 15:54 +0100, Ian Jackson wrote:> Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:"): > > Switch to a state machine parser since it''s easier to handle all these > > exotic cases without segfaulting. NULL physpaths are now allowed and a > > dodgy hack is introduced to skip over the "ioemu:" prefix for a > > virtpath. Also fixes a leak of buf2. > > I''m not convinced that it is clearer. It''s certainly a lot longer: > 132 lines added for 50 removed.It''s true that it''s longer but the nature of these types of parsers it''s a lot of very short lines :) I think it''s clearer than a correct strtok() + handling all errors and variations would be.> Perhaps we should just import a regexp parser and use that ? Really, > we should have a regexp or some other kind of declarative statement of > the syntax. > > Possible regexp parsers include pcre and flex. flex has the advantage > that we''re using it already so it''s just another line in the > Makefile.It''s your call, I know nothing of flex and its mysterious ways and my pcre skills are limited to basic text-editor-fu... I agree that flex probably makes the most sense. On the other hand whoever designed these formats seemed to want to make them difficult to parse. Since it''s all python I find myself wondering why they didn''t use a dictionary or a tuple. Gianni _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Aug-19 15:53 UTC
Re: [Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:
Gianni Tedesco (3P) writes ("Re: [Xen-devel] [PATCH]: xl: don''t
segfault parsing disk configs, support NULL physpath and
ioemu:"):> It''s true that it''s longer but the nature of these types
of parsers it''s
> a lot of very short lines :)
It adds 4164 characters and removes 1783. Discounting leading
whitespace it adds 2550 characters and removes 1085. However you
count it it''s between 2x and 3x as long :-).
I always think state machine based parsers are very hard to read by
eye. That''s why we have parser generators.
> I think it''s clearer than a correct strtok() + handling all errors
and
> variations would be.
Perhaps so.
> It''s your call, I know nothing of flex and its mysterious ways and
my
> pcre skills are limited to basic text-editor-fu... I agree that flex
> probably makes the most sense.
I''ll see if I can come up with a flex or pcre syntax that works and we
can see what people think of it.
> On the other hand whoever designed these formats seemed to want to make
> them difficult to parse. Since it''s all python I find myself
wondering
> why they didn''t use a dictionary or a tuple.
Just don''t go there :-).
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Aug-19 16:30 UTC
Re: [Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:
On Thu, 2010-08-19 at 16:53 +0100, Ian Jackson wrote:> Gianni Tedesco (3P) writes ("Re: [Xen-devel] [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:"): > > It''s true that it''s longer but the nature of these types of parsers it''s > > a lot of very short lines :) > > It adds 4164 characters and removes 1783. Discounting leading > whitespace it adds 2550 characters and removes 1085. However you > count it it''s between 2x and 3x as long :-). > > I always think state machine based parsers are very hard to read by > eye. That''s why we have parser generators. > > > I think it''s clearer than a correct strtok() + handling all errors and > > variations would be. > > Perhaps so. > > > It''s your call, I know nothing of flex and its mysterious ways and my > > pcre skills are limited to basic text-editor-fu... I agree that flex > > probably makes the most sense. > > I''ll see if I can come up with a flex or pcre syntax that works and we > can see what people think of it.Fair enough. While we''re on the subject why is there even a separate libxlutil.so? It''s not like the functionality is de-coupled and it seems to me like this parser stuff should be compiled right in to libxenlight.> > On the other hand whoever designed these formats seemed to want to make > > them difficult to parse. Since it''s all python I find myself wondering > > why they didn''t use a dictionary or a tuple. > > Just don''t go there :-).OK, I suppose I''d rather not then :) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel