Re: [Xen-devel] [PATCH] xl: improve vif2 parsing

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Fri, 2010-08-20 at 13:58 +0100, Andre Przywara wrote:
> Gianni Tedesco wrote:
> > On Fri, 2010-08-20 at 13:03 +0100, Andre Przywara wrote:
> >> Andre Przywara wrote:
> >>> Hi,
> >>>
> >>> vif2 parsing relies on counted strncmp() statements. Replace this
> >>> with a more robust automatic version.
> >> No, I didn't want to leave this as an exercise to the reader, I am just
> >> spoiled by git send-email, so forgot to attach the patch. Sorry!
> >>
> >>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >>>
> > 
> > Both patches look good to me.
> > 
> >>> Regards,
> >>> Andre.
> >>>
> >>> P.S. If you like this, I have seen at least two more instances of the 
> >>> same issue that could be improved this way.
> >>>
> > 
> > Can you say where?
> In main_networkattach() and main_network2attach().

You should absolutely sort this out. So I had already done a cleanup for
similar problem with PCI BDF's. You may want to introduce a
libxl_parse_netif() or something in libxl itself. You will also want a
destructor to prevent these from leaking. See idl.txt in the tools/libxl
root.

> > 
> > You should be aware that disk config parsing is undergoing a rewrite
> > already so lets not duplicate efforts on that one ;)
> I hoped you would say something like this. I see that parts of the code 
> has issues:
> * xl_cmdimpl.c is way too long (and will probably still have to grow)
> * code duplication in several parameter parsers
> * not reentrant safe (strtok instead of strtok_r)
> * Coding style (mostly 80 character limit)

To be honest I am not that concerned about length of xl_cmdimpl.c since
most of xl is straightfoward "parse a bit of config, pass results to a
libxl call" - but libxl.c could probably use splitting up to make it
easier to understand the subsystems within it. Splitting up
create_domain() is probably most urgent task in xl_cmdimpl.c.

Re-entrant safety would be nice, theoretically, but no threaded callers
exist (yet) to make it worth any large amount of effort.

Coding style / 80 char limit is a bit of a bummer but not sure how the
maintainers will feel about coding style patches. Probably be OK.

> So I was hoping that code cleanup was on someone's list, that saves me 
> from fixing many smaller things.

It is probably on several peoples lists but way below more substantive
things. For example I am trying to un-break multi-function PCI
passthrough for devices which share IRQ's - and for stubdoms - and
fixing error paths so that PCI subsystem isn't left in an unrecoverable
state when things go wrong - a real headache.

> Regards,
> Andre.
> 
> 
> Btw. I saw that cpuid= is still missing in libxl, I have a version 
> improved over the clumsy xm interface 90% ready, but will probably not 
> able to send it out still this week.

Yes, please patch and post. Also another thing is that 'uuid =' in the
config file is ignored. That's on my wishlist :)

I always look forward to seeing more patches.

Gianni


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