This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Andre Przywara <andre.przywara@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xl: improve vif2 parsing
From: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Date: Fri, 20 Aug 2010 14:34:29 +0100
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <Keir.Fraser@xxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Fri, 20 Aug 2010 07:20:32 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C6E7BE4.6090605@xxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C6E6E84.5020704@xxxxxxx> <4C6E6F20.3090405@xxxxxxx> <1282308116.3731.39.camel@xxxxxxxxxxxxxxxxxxxxxx> <4C6E7BE4.6090605@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

> > 
> > 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.


Xen-devel mailing list