[Libvir] [PATCH] Fix the check of <cpumap> in virsh vcpupin
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@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); ----------------------------------------------------------------------
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
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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
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
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@redhat.com> "Re: [Libvir] [PATCH] Fix the check of <cpumap> in virsh vcpupin" ""Richard W.M. Jones" <rjones@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
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
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel Veillard -
Masayuki Sunou -
Richard W.M. Jones