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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 ;) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel