[Libvir] [PATCH] Fix string handling in virDomain{Get, Set}SchedulerParameters
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters. Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match". Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Hi, Rich Thank you for your review and fix. +1 Thanks Atsushi SAKAI "Richard W.M. Jones" <rjones@redhat.com> wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
Rich.
-- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Atsushi SAKAI wrote:
Hi, Rich
Thank you for your review and fix. +1
I actually got around to using these functions, and apart from the string problem they work fine. I'm just adding remote support now. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 22, 2007 at 10:30:37AM +0100, Richard W.M. Jones wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
yeah the strncmp use is vrong, it should be fixed. But I dislike the change from strncpy to strcpy. I agree that affecting local variables with fixed strings is not nice, but as we try to chase strcpy call out of the code those will show up, le'ts use strncpy(..., "weight", 6) instead. Okay ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Fri, Jun 22, 2007 at 10:30:37AM +0100, Richard W.M. Jones wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
yeah the strncmp use is vrong, it should be fixed. But I dislike the change from strncpy to strcpy. I agree that affecting local variables with fixed strings is not nice, but as we try to chase strcpy call out of the code those will show up, le'ts use strncpy(..., "weight", 6) instead. Okay ?
I don't understand ... This was the original problem, namely that the field was being initialised to the string followed by garbage (without a terminating '\0'): (gdb) print params[1] $5 = { field = "cap\205�\177\000\000�pԪ�*\000\000\000\000\000\000\000\000\000\000\003ˠ.4\000\000\000\001", '\0' <repeats 23 times>, "`\231\024/4\000\000\000\200Q�\205�\177\000\000�F@\000\000\000\000", type = 2, value = {i = 0, ui = 0, l = 140733193388032, ul = 140733193388032, d = 6.9531436082559078e-310, b = 0 '\0'}} On the other hand if you meant strncpy (field, "weight", VIR_DOMAIN_SCHED_FIELD_LENGTH), followed by field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0' then I'd agree. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 22, 2007 at 10:53:22AM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
On Fri, Jun 22, 2007 at 10:30:37AM +0100, Richard W.M. Jones wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
yeah the strncmp use is vrong, it should be fixed. But I dislike the change from strncpy to strcpy. I agree that affecting local variables with fixed strings is not nice, but as we try to chase strcpy call out of the code those will show up, le'ts use strncpy(..., "weight", 6) instead. Okay ?
I don't understand ... This was the original problem, namely that the field was being initialised to the string followed by garbage (without a terminating '\0'):
ohh, then count the terminating 0 in, I just would prefer to avoid using strcpy to simplify the maintainance task of chasing them.
On the other hand if you meant strncpy (field, "weight", VIR_DOMAIN_SCHED_FIELD_LENGTH), followed by field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0' then I'd agree.
that works too. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
"Richard W.M. Jones" <rjones@redhat.com> wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
Hi Rich, I agree, but prefer to avoid direct use of strcmp altogether. I use this definition: #define STREQ(a, b) (strcmp (a, b) == 0) Then all uses are either STREQ(a, b) or ! STREQ(a, b)
Jim Meyering wrote:
"Richard W.M. Jones" <rjones@redhat.com> wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
Hi Rich,
I agree, but prefer to avoid direct use of strcmp altogether. I use this definition:
#define STREQ(a, b) (strcmp (a, b) == 0)
Then all uses are either STREQ(a, b) or ! STREQ(a, b)
Let's try this updated patch. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 22, 2007 at 11:03:45AM +0100, Richard W.M. Jones wrote:
Jim Meyering wrote:
"Richard W.M. Jones" <rjones@redhat.com> wrote:
This small patch fixes some bugs in the handling of the field string in virDomainGetSchedulerParameters and makes a similar pre-emptive fix to virDomainSetSchedulerParameters.
Also, please don't use !strcmp(a,b), because it confuses me. Better is to write strcmp(a,b) == 0 to mean "strings match" and strcmp(a,b) != 0 to mean "strings don't match".
Hi Rich,
I agree, but prefer to avoid direct use of strcmp altogether. I use this definition:
#define STREQ(a, b) (strcmp (a, b) == 0)
Then all uses are either STREQ(a, b) or ! STREQ(a, b)
Let's try this updated patch.
+1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Let's try this updated patch.
Hi Rich, It'd be nice to factor those "cap" and "weight" literal strings all the way out, so each is defined in exactly one place. Either with a file-scoped const declaration, e.g., static const char *cap_str = "cap"; or a cpp define: #define CAP_STR "cap"; Then, there's no risk (however small) of changing one of the literal strings without changing its mate.
Index: src/xen_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xen_internal.c,v retrieving revision 1.79 diff -u -p -r1.79 xen_internal.c --- src/xen_internal.c 6 Jun 2007 12:24:31 -0000 1.79 +++ src/xen_internal.c 22 Jun 2007 10:01:29 -0000 @@ -1071,8 +1071,6 @@ xenHypervisorGetSchedulerParameters(virD ... + strncpy (params[0].field, "weight", VIR_DOMAIN_SCHED_FIELD_LENGTH); ... + strncpy (params[1].field, "cap", VIR_DOMAIN_SCHED_FIELD_LENGTH); ... + if (STREQ (params[i].field, "weight") && ... + } else if (STREQ (params[i].field, "cap") &&
Jim Meyering wrote:
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Let's try this updated patch.
Hi Rich,
It'd be nice to factor those "cap" and "weight" literal strings all the way out, so each is defined in exactly one place. Either with a file-scoped const declaration, e.g., static const char *cap_str = "cap"; or a cpp define: #define CAP_STR "cap";
Then, there's no risk (however small) of changing one of the literal strings without changing its mate.
I've applied the patch + Jim's suggestion above. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (4)
-
Atsushi SAKAI -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones