[PATCH] xl: improve vif2 parsing

[PATCH] xl: improve vif2 parsing

[Andre Przywara]

Hi,

vif2 parsing relies on counted strncmp() statements. Replace this
with a more robust automatic version.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

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.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[PATCH] xl: improve vif2 parsing

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

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@amd.com>
> 
> 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.
> 


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: xl-improve-vif2-parsing.patch --]
[-- Type: text/x-patch, Size: 3468 bytes --]

>From 7b34567697aaf8b8acf251d63519d08a5fb7b646 Mon Sep 17 00:00:00 2001
From: Andre Przywara <andre.przywara@amd.com>
Date: Fri, 20 Aug 2010 01:01:43 +0200
Subject: [PATCH 3/4] xl: improve vif2 parsing

vif2 parsing relies on counted strncmp() statements. Replace this
with a more robust automatic version.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 tools/libxl/xl_cmdimpl.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c291879..1ddaab9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -860,28 +860,33 @@ skip:
             init_net2_info(net2, d_config->num_vif2s);
 
             for (p = strtok(buf2, ","); p; p = strtok(NULL, ",")) {
+                char* val;
                 while (isblank(*p))
                     p++;
-                if (!strncmp("front_mac=", p, 10)) {
-                    libxl_strtomac(p + 10, net2->front_mac);
-                } else if (!strncmp("back_mac=", p, 9)) {
-                    libxl_strtomac(p + 9, net2->back_mac);
-                } else if (!strncmp("backend=", p, 8)) {
-                    domain_qualifier_to_domid(p + 8, &net2->backend_domid, 0);
-                } else if (!strncmp("trusted=", p, 8)) {
-                    net2->trusted = (*(p + 8) == '1');
-                } else if (!strncmp("back_trusted=", p, 13)) {
-                    net2->back_trusted = (*(p + 13) == '1');
-                } else if (!strncmp("bridge=", p, 7)) {
-                    net2->bridge = strdup(p + 13);
-                } else if (!strncmp("filter_mac=", p, 11)) {
-                    net2->filter_mac = (*(p + 11) == '1');
-                } else if (!strncmp("front_filter_mac=", p, 17)) {
-                    net2->front_filter_mac = (*(p + 17) == '1');
-                } else if (!strncmp("pdev=", p, 5)) {
-                    net2->pdev = strtoul(p + 5, NULL, 10);
-                } else if (!strncmp("max_bypasses=", p, 13)) {
-                    net2->max_bypasses = strtoul(p + 13, NULL, 10);
+                val = strchr(p, '=');
+                if (val == NULL)
+                    continue;
+                *val++ = 0;
+                if (!strcmp("front_mac", p)) {
+                    libxl_strtomac(val, net2->front_mac);
+                } else if (!strcmp("back_mac", p)) {
+                    libxl_strtomac(val, net2->back_mac);
+                } else if (!strcmp("backend", p)) {
+                    domain_qualifier_to_domid(val, &net2->backend_domid, 0);
+                } else if (!strcmp("trusted", p)) {
+                    net2->trusted = (*val == '1');
+                } else if (!strcmp("back_trusted", p)) {
+                    net2->back_trusted = (*val == '1');
+                } else if (!strcmp("bridge", p)) {
+                    net2->bridge = strdup(val);
+                } else if (!strcmp("filter_mac", p)) {
+                    net2->filter_mac = (*val == '1');
+                } else if (!strcmp("front_filter_mac", p)) {
+                    net2->front_filter_mac = (*val == '1');
+                } else if (!strcmp("pdev", p)) {
+                    net2->pdev = strtoul(val, NULL, 10);
+                } else if (!strcmp("max_bypasses", p)) {
+                    net2->max_bypasses = strtoul(val, NULL, 10);
                 }
             }
             free(buf2);
-- 
1.6.4


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] xl: improve vif2 parsing

[Gianni Tedesco]

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@amd.com>
> >

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?

You should be aware that disk config parsing is undergoing a rewrite
already so lets not duplicate efforts on that one ;)

Re: [PATCH] xl: improve vif2 parsing

[Andre Przywara]

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@amd.com>
>>>
> 
> 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 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)

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

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.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712

Re: [PATCH] xl: improve vif2 parsing

[Gianni Tedesco]

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@amd.com>
> >>>
> > 
> > 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

Re: [PATCH] xl: improve vif2 parsing

[Christoph Egger]

On Friday 20 August 2010 15:34:29 Gianni Tedesco wrote:
> 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@amd.com>
> > >
> > > 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.

Yes and OS-dependent code still needs to be sorted out.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl: improve vif2 parsing

[Gianni Tedesco]

On Fri, 2010-08-20 at 15:28 +0100, Christoph Egger wrote:
> On Friday 20 August 2010 15:34:29 Gianni Tedesco wrote:
> > 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@amd.com>
> > > >
> > > > 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.
> 
> Yes and OS-dependent code still needs to be sorted out.

Indeed, the other changes I mentioned (plus correct safety checks) may
change the structure of the code so much that the OS-specific interfaces
will change substantially. For that reason it's at the bottom of the
list right now.

Gianni

Re: [PATCH] xl: improve vif2 parsing

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH] xl: improve vif2 parsing"):
> 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!

This looks nice to me but it doesn't apply to xen-unstable staging
tip.  Can you take a look and either rebase or see if you can see what
the problem is ?  (Perhaps it has been mangled by your mailer.)

Thanks,
Ian.

Re: [PATCH] xl: improve vif2 parsing

[Stefano Stabellini]

On Fri, 20 Aug 2010, Gianni Tedesco (3P) wrote:
> 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.

Yes, create_domain should be refactored and some code should be move to
libxl: we cannot expect all the possible libxl callers to replicate the
complexity of create_domain.


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

I would be happy to apply code style patches, however I have to say that
greater than 80 chars lines don't bother me.

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

Re: [PATCH] xl: improve vif2 parsing

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH] xl: improve vif2 parsing"):
>> 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!
> 
> This looks nice to me but it doesn't apply to xen-unstable staging
> tip.  Can you take a look and either rebase or see if you can see what
> the problem is ?  (Perhaps it has been mangled by your mailer.)
No, it just interfered and thus depended on the bug-fix I sent one mail
earlier. So, please, first apply "[PATCH] xl: fix strtok() call in vif2
parsing", then try the patch again. This worked for me on top of
staging:22054.
I think the former bug-fix should be applied in every case, but I attach
the non-dependent version of the patch for the sake of completeness. But
this one still carries the old bug!

Regards,
Andre.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: xl-improve-vif2-parsing_buggy_context.patch --]
[-- Type: text/plain, Size: 2965 bytes --]

diff -r 81503caeb439 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Sun Aug 22 09:53:51 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Aug 23 00:43:22 2010 +0200
@@ -884,28 +884,33 @@
             init_net2_info(net2, d_config->num_vif2s);
 
             for (p = strtok(buf2, ","); p; p = strtok(buf2, ",")) {
+                char* val;
                 while (isblank(*p))
                     p++;
-                if (!strncmp("front_mac=", p, 10)) {
-                    libxl_strtomac(p + 10, net2->front_mac);
-                } else if (!strncmp("back_mac=", p, 9)) {
-                    libxl_strtomac(p + 9, net2->back_mac);
-                } else if (!strncmp("backend=", p, 8)) {
-                    domain_qualifier_to_domid(p + 8, &net2->backend_domid, 0);
-                } else if (!strncmp("trusted=", p, 8)) {
-                    net2->trusted = (*(p + 8) == '1');
-                } else if (!strncmp("back_trusted=", p, 13)) {
-                    net2->back_trusted = (*(p + 13) == '1');
-                } else if (!strncmp("bridge=", p, 7)) {
-                    net2->bridge = strdup(p + 13);
-                } else if (!strncmp("filter_mac=", p, 11)) {
-                    net2->filter_mac = (*(p + 11) == '1');
-                } else if (!strncmp("front_filter_mac=", p, 17)) {
-                    net2->front_filter_mac = (*(p + 17) == '1');
-                } else if (!strncmp("pdev=", p, 5)) {
-                    net2->pdev = strtoul(p + 5, NULL, 10);
-                } else if (!strncmp("max_bypasses=", p, 13)) {
-                    net2->max_bypasses = strtoul(p + 13, NULL, 10);
+                val = strchr(p, '=');
+                if (val == NULL)
+                    continue;
+                *val++ = 0;
+                if (!strcmp("front_mac", p)) {
+                    libxl_strtomac(val, net2->front_mac);
+                } else if (!strcmp("back_mac", p)) {
+                    libxl_strtomac(val, net2->back_mac);
+                } else if (!strcmp("backend", p)) {
+                    domain_qualifier_to_domid(val, &net2->backend_domid, 0);
+                } else if (!strcmp("trusted", p)) {
+                    net2->trusted = (*val == '1');
+                } else if (!strcmp("back_trusted", p)) {
+                    net2->back_trusted = (*val == '1');
+                } else if (!strcmp("bridge", p)) {
+                    net2->bridge = strdup(val);
+                } else if (!strcmp("filter_mac", p)) {
+                    net2->filter_mac = (*val == '1');
+                } else if (!strcmp("front_filter_mac", p)) {
+                    net2->front_filter_mac = (*val == '1');
+                } else if (!strcmp("pdev", p)) {
+                    net2->pdev = strtoul(val, NULL, 10);
+                } else if (!strcmp("max_bypasses", p)) {
+                    net2->max_bypasses = strtoul(val, NULL, 10);
                 }
             }
             free(buf2);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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