Re: [PATCH 0/4] xl: consolidate adhoc parsers

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-02-16 at 23:20 +0000, Wei Liu wrote:
> On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote:
> > Wei Liu wrote:
> > > This patch series consolidates adhoc parsers in xl.
> > 
> > Hi Wei,
> > 
> > I never tested or reviewed this series after seeing Ian's comments. Did
> > you have
> > time to work on a V2? Or did I miss a V2? :-) Let me know if I can
> > help.
> > 
> 
> Sorry, this series fell through the crack -- it was only the work of
> one afternoon when I had some free cycles. Now I'm busy with other
> stuff and haven't had the time to pick it up again.
> 
> One thing I'm not entirely happy with writing this in plain C is that
> there is subtle semantics difference from the ones generated by
> flex/bison even if they expose similar APIs. For example, the
> functions to parse disk spec won't allow you to set same attribute
> twice, while the work present in this function allows you to do
> that. It's cumbersome to implement the same functionality with open
> coding in C IMHO.

Take care -- the xlu disk parser has a lot of additional complexity and
scaffolding to support the "legacy" xm style device specifications with
positional parameters and strange (ignored by xl) prefixes, trying to merge
that with other device types is likely to result in unwanted/unmanageable
complexity IMHO.

I'm also not sure that switching to flex+bison for the rest is going to
solve the problems you mention above (disallowing setting something twice
etc). flex+bison will take care nicely of things like tokenising and
parsing the input (i.e. it is a nice replacement for strtok and friends),
but ultimately the keys and values need to be exposed to the user somehow
i.e. put into some data structure.

That data structure could either be completely abstract (i.e. just provide
"lookup by key" functionality, via some sort of hash or list etc, which is
basically what the xlu cfg stuff does) or device type specific (i.e. fills
in a given struct based on a knowledge of the value keys for that struct,
which is what the disk stuff does) and it's in that code where you would
need to handle things like what to do with repeated keys.

That is essentially just done by the C snippets in the bison rules or the
scaffolding surrounding the bison, it's not something that bison inherently
deals with.

So ultimately you need to deal with the question of what the semantics of
setting a given key a second time are (error, take the first value, take
the last value, per-key settings etc) in C code, even if you are using
bison. That's slightly easier if you have a parser per device type, since
you can just check the specific field in the specific struct for being
unset or not before making a choice, but even in abstract code it's just a
case of checking in the list/table/hash you are constructing during parsing
for the key and reacting accordingly, but in both cases you are doing so in
C not in "bison".

Ian.