3:30 a.m.
Hi
Because virsh vcpupin does not check a form of <cpumap>,
when non-numerical letters are set, it does not become an error.
This patch fixes it so that it become an error when non-numerical
letters are set.
Signed-off-by: Masayuki Sunou <fj1826dm(a)aa.jp.fujitsu.com>
Thanks,
Masayuki Sunou.
----------------------------------------------------------------------
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.84
diff -u -p -r1.84 virsh.c
--- src/virsh.c 15 Jun 2007 15:24:20 -0000 1.84
+++ src/virsh.c 18 Jun 2007 07:02:31 -0000
@@ -1530,6 +1530,7 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm
int vcpufound = 0;
unsigned char *cpumap;
int cpumaplen;
+ int i;
if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
return FALSE;
@@ -1563,6 +1564,22 @@ cmdVcpupin(vshControl * ctl, vshCmd * cm
return FALSE;
}
+ for(i = 0; cpulist[i]; i++) {
+ if ((i == 0) || (i == strlen(cpulist)-1)) {
+ if (!isdigit(cpulist[i])) {
+ vshError(ctl, FALSE, _("Invalid format '%s'."),
cpulist);
+ virDomainFree(dom);
+ return FALSE;
+ }
+ } else {
+ if(!isdigit(cpulist[i]) && cpulist[i] != ',') {
+ vshError(ctl, FALSE, _("Invalid format '%s'."),
cpulist);
+ virDomainFree(dom);
+ return FALSE;
+ }
+ }
+ }
+
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
cpumap = vshCalloc(ctl, 1, cpumaplen);
----------------------------------------------------------------------
6:02 a.m.
Masayuki Sunou wrote:
Hi
Because virsh vcpupin does not check a form of <cpumap>,
when non-numerical letters are set, it does not become an error.
This patch fixes it so that it become an error when non-numerical
letters are set.
You mean cpulist instead of cpumap? Your patch doesn't deal with
strings like "1,,,,3".
<rant> It's 2007 - we should not be writing any more software in C.
</rant>
There seem to be another problem with the documentation of virsh vcpupin
too. It's documented as 'virsh vcpupin <domain>' yet takes two other
parameters, so should really be 'virsh vcpupin <domain> <vcpu>
<cpulist>'.
The attached patch (not applied) should fix all of the 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
8:05 a.m.
On Mon, Jun 18, 2007 at 12:02:01PM +0100, Richard W.M. Jones wrote:
<rant> It's 2007 - we should not be writing any more
software in C. </rant>
You're right, I really need to write a javascript interpreter and
embbed it in libxml2 so we can rewrite all the upper layers.
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/
8:15 a.m.
Daniel Veillard wrote:
On Mon, Jun 18, 2007 at 12:02:01PM +0100, Richard W.M. Jones wrote:
> <rant> It's 2007 - we should not be writing any more software in C.
</rant>
You're right, I really need to write a javascript interpreter and
embbed it in libxml2 so we can rewrite all the upper layers.
Nah, dynamic languages suck even more. Particularly interpreted ones.
let ids =
try List.map int_of_string (String.nsplit cpulist ",")
with Failure "int_of_string" ->
raise (VirshError (cpulist ": invalid format"))
That's the same code (actually better - not only does it do the check,
but it converts the CPU numbers to a list of integers for you), safer,
the same speed as the C code, and a factor of 10 shorter.
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
11:20 a.m.
Richard W.M. Jones wrote:
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.85
diff -u -r1.85 virsh.c
--- src/virsh.c 18 Jun 2007 08:33:08 -0000 1.85
+++ src/virsh.c 18 Jun 2007 11:00:59 -0000
@@ -1505,7 +1505,7 @@
* "vcpupin" command
*/
static vshCmdInfo info_vcpupin[] = {
- {"syntax", "vcpupin <domain>"},
+ {"syntax", "vcpupin <domain> <vcpu>
<cpulist>"},
{"help", gettext_noop("control domain vcpu affinity")},
{"desc", gettext_noop("Pin domain VCPUs to host physical
CPUs.")},
{NULL, NULL}
@@ -1530,6 +1530,8 @@
int vcpufound = 0;
unsigned char *cpumap;
int cpumaplen;
+ int i;
+ enum { expect_num, expect_num_or_comma } state;
if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
return FALSE;
@@ -1563,6 +1565,42 @@
return FALSE;
}
+ /* Check that the cpulist parameter is a comma-separated list of
+ * numbers and give an intelligent error message if not.
+ */
+ if (cpulist[0] == '\0') {
+ vshError(ctl, FALSE, _("cpulist: Invalid format. Empty string."));
+ virDomainFree (dom);
+ return FALSE;
+ }
+
+ state = expect_num;
+ for (i = 0; cpulist[i]; i++) {
+ switch (state) {
+ case expect_num:
+ if (!isdigit (cpulist[i])) {
+ vshError( ctl, FALSE, _("cpulist: %s: Invalid format. Expecting
digit at position %d (near '%c')."), cpulist, i, cpulist[i]);
+ virDomainFree (dom);
+ return FALSE;
+ }
+ state = expect_num_or_comma;
+ break;
+ case expect_num_or_comma:
+ if (cpulist[i] == ',')
+ state = expect_num;
+ else if (!isdigit (cpulist[i])) {
+ vshError(ctl, FALSE, _("cpulist: %s: Invalid format. Expecting
digit or comma at position %d (near '%c')."), cpulist, i, cpulist[i]);
+ virDomainFree (dom);
+ return FALSE;
+ }
+ }
+ }
+ if (state == expect_num) {
+ vshError(ctl, FALSE, _("cpulist: %s: Invalid format. Trailing comma at
position %d."), cpulist, i);
+ virDomainFree (dom);
+ return FALSE;
+ }
+
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
cpumap = vshCalloc(ctl, 1, cpumaplen);
I want to commit this one if we're all happy with it ...?
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
3:37 a.m.
Hi Rich
Thank you for a reviewing and improvement.
It looks good to me.
I want to commit this one if we're all happy with it ...?
If this patch is applied, our team member become happy.
Thanks
Masayuki Sunou
In message <4676B0BD.4030307(a)redhat.com>
"Re: [Libvir] [PATCH] Fix the check of <cpumap> in virsh vcpupin"
""Richard W.M. Jones" <rjones(a)redhat.com>" wrote:
> Richard W.M. Jones wrote:
> > Index: src/virsh.c
> > ===================================================================
> > RCS file: /data/cvs/libvirt/src/virsh.c,v
> > retrieving revision 1.85
> > diff -u -r1.85 virsh.c
> > --- src/virsh.c 18 Jun 2007 08:33:08 -0000 1.85
> > +++ src/virsh.c 18 Jun 2007 11:00:59 -0000
> > @@ -1505,7 +1505,7 @@
> > * "vcpupin" command
> > */
> > static vshCmdInfo info_vcpupin[] = {
> > - {"syntax", "vcpupin <domain>"},
> > + {"syntax", "vcpupin <domain> <vcpu>
<cpulist>"},
> > {"help", gettext_noop("control domain vcpu
affinity")},
> > {"desc", gettext_noop("Pin domain VCPUs to host physical
CPUs.")},
> > {NULL, NULL}
> > @@ -1530,6 +1530,8 @@
> > int vcpufound = 0;
> > unsigned char *cpumap;
> > int cpumaplen;
> > + int i;
> > + enum { expect_num, expect_num_or_comma } state;
> >
> > if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
> > return FALSE;
> > @@ -1563,6 +1565,42 @@
> > return FALSE;
> > }
> >
> > + /* Check that the cpulist parameter is a comma-separated list of
> > + * numbers and give an intelligent error message if not.
> > + */
> > + if (cpulist[0] == '\0') {
> > + vshError(ctl, FALSE, _("cpulist: Invalid format. Empty
string."));
> > + virDomainFree (dom);
> > + return FALSE;
> > + }
> > +
> > + state = expect_num;
> > + for (i = 0; cpulist[i]; i++) {
> > + switch (state) {
> > + case expect_num:
> > + if (!isdigit (cpulist[i])) {
> > + vshError( ctl, FALSE, _("cpulist: %s: Invalid format.
Expecting digit at position %d (near '%c')."), cpulist, i, cpulist[i]);
> > + virDomainFree (dom);
> > + return FALSE;
> > + }
> > + state = expect_num_or_comma;
> > + break;
> > + case expect_num_or_comma:
> > + if (cpulist[i] == ',')
> > + state = expect_num;
> > + else if (!isdigit (cpulist[i])) {
> > + vshError(ctl, FALSE, _("cpulist: %s: Invalid format.
Expecting digit or comma at position %d (near '%c')."), cpulist, i,
cpulist[i]);
> > + virDomainFree (dom);
> > + return FALSE;
> > + }
> > + }
> > + }
> > + if (state == expect_num) {
> > + vshError(ctl, FALSE, _("cpulist: %s: Invalid format. Trailing
comma at position %d."), cpulist, i);
> > + virDomainFree (dom);
> > + return FALSE;
> > + }
> > +
> > cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
> > cpumap = vshCalloc(ctl, 1, cpumaplen);
>
I want to commit this one if we're all happy with it ...?
> 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
>
7:19 a.m.
Masayuki:
That's checked in now. Thank you for your contribution to libvirt.
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:25 a.m.
On Mon, Jun 18, 2007 at 05:20:13PM +0100, Richard W.M. Jones wrote:
Richard W.M. Jones wrote:
>Index: src/virsh.c
>===================================================================
>RCS file: /data/cvs/libvirt/src/virsh.c,v
>retrieving revision 1.85
>diff -u -r1.85 virsh.c
>--- src/virsh.c 18 Jun 2007 08:33:08 -0000 1.85
>+++ src/virsh.c 18 Jun 2007 11:00:59 -0000
>@@ -1505,7 +1505,7 @@
> * "vcpupin" command
> */
> static vshCmdInfo info_vcpupin[] = {
>- {"syntax", "vcpupin <domain>"},
>+ {"syntax", "vcpupin <domain> <vcpu>
<cpulist>"},
> {"help", gettext_noop("control domain vcpu affinity")},
> {"desc", gettext_noop("Pin domain VCPUs to host physical
CPUs.")},
> {NULL, NULL}
>@@ -1530,6 +1530,8 @@
> int vcpufound = 0;
> unsigned char *cpumap;
> int cpumaplen;
>+ int i;
>+ enum { expect_num, expect_num_or_comma } state;
>
> if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
> return FALSE;
>@@ -1563,6 +1565,42 @@
> return FALSE;
> }
>
>+ /* Check that the cpulist parameter is a comma-separated list of
>+ * numbers and give an intelligent error message if not.
>+ */
>+ if (cpulist[0] == '\0') {
>+ vshError(ctl, FALSE, _("cpulist: Invalid format. Empty
string."));
>+ virDomainFree (dom);
>+ return FALSE;
>+ }
>+
>+ state = expect_num;
>+ for (i = 0; cpulist[i]; i++) {
>+ switch (state) {
>+ case expect_num:
>+ if (!isdigit (cpulist[i])) {
>+ vshError( ctl, FALSE, _("cpulist: %s: Invalid format.
>Expecting digit at position %d (near '%c')."), cpulist, i, cpulist[i]);
>+ virDomainFree (dom);
>+ return FALSE;
>+ }
>+ state = expect_num_or_comma;
>+ break;
>+ case expect_num_or_comma:
>+ if (cpulist[i] == ',')
>+ state = expect_num;
>+ else if (!isdigit (cpulist[i])) {
>+ vshError(ctl, FALSE, _("cpulist: %s: Invalid format.
>Expecting digit or comma at position %d (near '%c')."), cpulist, i,
>cpulist[i]);
>+ virDomainFree (dom);
>+ return FALSE;
>+ }
>+ }
>+ }
>+ if (state == expect_num) {
>+ vshError(ctl, FALSE, _("cpulist: %s: Invalid format. Trailing
>comma at position %d."), cpulist, i);
>+ virDomainFree (dom);
>+ return FALSE;
>+ }
>+
> cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
> cpumap = vshCalloc(ctl, 1, cpumaplen);
I want to commit this one if we're all happy with it ...?
Yes please :-)
Our documentation for virsh is lagging behind I'm afraid, and the help
command doesn't really replace a complete man page or a getting started
kind of tutorial.
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/
6369
days inactive
6370
days old
7 comments
3 participants
participants (3)
-
Daniel Veillard -
Masayuki Sunou -
Richard W.M. Jones