From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH] libxl: new xlu_disk_parse function
Date: Thu, 31 Mar 2011 18:30:19 +0100
Message-ID: <19860.47659.19132.816771@mariner.uk.xensource.com>
References: <1300988989-18305-1-git-send-email-ian.jackson@eu.citrix.com>
	<1300988989-18305-2-git-send-email-ian.jackson@eu.citrix.com>
	<1301333477.1691.7.camel@qabil.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <1301333477.1691.7.camel@qabil.uk.xensource.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Gianni Tedesco <gianni.tedesco@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org

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.