xl: improve vif2 parsing
PATCH 1/1 xl: improve vif2 parsing
Hi,
vif2 parsing relies on counted strncmp() statements. Replace this with a more robust automatic version.
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.
Signature · 3 lines
Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12
PATCH 1/1 xl: improve vif2 parsing
Quoted 4 lines · Andre Przywara
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!
Quoted 8 lines
Signed-off-by Andre Przywara <[email protected]>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.
Signature · 3 lines
Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12
Quoted 1 line
From
7b34567697aaf8b8acf251d63519d08a5fb7b646Mon Sep 17 00:00:00 2001
From: Andre Przywara <[email protected]>
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.
--- tools/libxl/xl_cmdimpl.c | 45 +++++++++++++++++++++++++-------------------- 1 files changed, 25 insertions(+), 20 deletions(-)
tools/libxl/xl_cmdimpl.c +25 -20
@@ -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);
Signature · 1 line
1.6.4
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 12 lines · Andre Przywara
Quoted 4 lines · Andre Przywara
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!
Quoted 3 lines
Signed-off-by Andre Przywara <[email protected]>
Both patches look good to me.
Quoted 6 lines
Quoted 6 lines
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 ;)
Quoted 14 lines · Gianni Tedesco
Quoted 10 lines · Andre Przywara
Quoted 4 lines · Andre Przywara
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!
Quoted 2 lines
Signed-off-by Andre Przywara <[email protected]>Both patches look good to me.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
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().
Quoted 3 lines
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.
Signature · 3 lines
Andre Przywara AMD-OSRC (Dresden) Tel: x29712
Quoted 18 lines · Andre Przywara
Quoted 17 lines · Gianni Tedesco
Quoted 10 lines · Andre Przywara
Quoted 4 lines · Andre Przywara
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!
Quoted 2 lines
Signed-off-by Andre Przywara <[email protected]>Both patches look good to me.
Quoted 3 lines
Quoted 3 lines
Regards,
Andre.
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
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()andmain_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.
Quoted 9 lines
Quoted 3 lines
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.cis way too long (and will probably still have to grow)
* code duplication in several parameter parsers
* not reentrant safe (strtok instead ofstrtok_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.
Quoted 2 lines
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.
Quoted 7 lines
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
Quoted 22 lines · On Friday 20 August 2010 15:34:29 Gianni Tedesco wrote:
Quoted 21 lines · Andre Przywara
Quoted 20 lines · Gianni Tedesco
Quoted 10 lines · Andre Przywara
Quoted 4 lines · Andre Przywara
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!
Quoted 1 line
Signed-off-by Andre Przywara <[email protected]>Both patches look good to me.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Can you say where?
In
main_networkattach()andmain_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
Signature · 6 lines
---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
Quoted 23 lines · Christoph Egger
Quoted 22 lines · On Friday 20 August 2010 15:34:29 Gianni Tedesco wrote:
Quoted 21 lines · Andre Przywara
Quoted 20 lines · Gianni Tedesco
Quoted 10 lines · Andre Przywara
Quoted 4 lines · Andre Przywara
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!
Quoted 1 line
Signed-off-by Andre Przywara <[email protected]>Both patches look good to me.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Can you say where?
In
main_networkattach()andmain_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
Quoted 5 lines · On Fri, 20 Aug 2010, Gianni Tedesco (3P) wrote:
To be honest I am not that concerned about length of
xl_cmdimpl.csince most of xl is straightfoward "parse a bit of config, pass results to a libxl call" - butlibxl.ccould probably use splitting up to make it easier to understand the subsystems within it. Splitting upcreate_domain()is probably most urgent task inxl_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.
Quoted 3 lines
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.
Quoted 9 lines
Quoted 3 lines
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
Andre Przywara writes ("[Xen-devel] [PATCH] xl: improve vif2 parsing"):
Quoted 2 lines
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.
Quoted 7 lines · Ian Jackson
Andre Przywara writes ("[Xen-devel] [PATCH] xl: improve vif2 parsing"):
Quoted 2 lines
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.
Signature · 3 lines
Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12
tools/libxl/xl_cmdimpl.c +25 -20
@@ -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);
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel