Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Thu, 2011-03-31 at 18:30 +0100, Ian Jackson wrote:
> 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.

Well it is designed so that the pointer arithmetic parts are the main
loop which doesn't need altering. Within the loop it's just a matter of
doing state transitions and that is the essence of the parser. Just a
bit of care and testing when modifying it is all that's required - it's
not *that* subtle.

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel