Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse
function"):
> On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > + return;
>
> case 1: ??
Deliberately omitted.
> > + case 2:
> > + case 3:
> > + case 4:
> > + break;
>
> Or does it belong here? In which case aborting on a parse error is bad
> juju.
> case 1:
> > + default:
> > + abort();
I could add it there for clarity. The regexp will always match
capturing with 2, 3 or 4 parens. None of the other errors from
dfa_exec are applicable. So anything other than 2,3,4 or "did not
match" is due to a bug in the code, not merely bogus input. Perhaps
this should be mentioned in a comment.
> Hmm, I'm not sure this is nicer than the code it's replacing, it's
> certainly a lot longer. I don't like the idea of it being full of things
> comparing for ",w" or ":cdrom" etc, rather than tokenising it fully and
> evaluating what the tokens are separately.
Perhaps you're right. Unfortunately the nasty multi-level nature of
this parsing problem makes this a bit awkward.
But I think I can remove the delimiters into the regexp which perhaps
will help.
> > +raw: { DSET(format,FORMAT,RAW); }
> > +qcow: { DSET(format,FORMAT,QCOW); }
> > +qcow2: { DSET(format,FORMAT,QCOW2); }
> > +vhd: { DSET(format,FORMAT,QCOW2); }
> > +
> > +phy: { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,PHY); }
> > +file: { DSET(format,FORMAT,RAW); DSET(backend,BACKEND,TAP); }
> > +tapdisk:|tap2?: { DSET(backend,BACKEND,TAP); }
> > +aio: { }
> > +ioemu: { }
>
> This bit is quite nice though. We could probably just tidy up the
> existing parser using arrays of values and things rather than a lot of
> if/else statements though.
I wanted to avoid parsing with pointer arithmetic, which is very easy
to write bugs in - particularly when new features are added.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home