4:30 a.m.
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
4:33 a.m.
Hi, Rich
Thank you for your review and fix.
+1
Thanks
Atsushi SAKAI
"Richard W.M. Jones" <rjones(a)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
4:41 a.m.
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
4:43 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:53 a.m.
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
5:09 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:52 a.m.
"Richard W.M. Jones" <rjones(a)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)
5:03 a.m.
Jim Meyering wrote:
"Richard W.M. Jones" <rjones(a)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
5:10 a.m.
On Fri, Jun 22, 2007 at 11:03:45AM +0100, Richard W.M. Jones wrote:
Jim Meyering wrote:
>"Richard W.M. Jones" <rjones(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:19 a.m.
"Richard W.M. Jones" <rjones(a)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") &&
6:43 a.m.
Jim Meyering wrote:
"Richard W.M. Jones" <rjones(a)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
6328
days inactive
6328
days old
10 comments
4 participants
participants (4)
-
Atsushi SAKAI -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones