Re: [PATCH] domain controller index check

[Ján Tomko <jtomko@xxxxxxxxxx>]

On 07/09/2013 06:13 AM, Jincheng Miao wrote:
> The index of the controller should not be limited in [zero, INT_MAX].
> So use virStrToLong_ui() and check the limit of the controller
> index in virDomainControllerDefParseXML().
> ---
>  src/conf/domain_conf.c  | 6 +++---
>  src/conf/domain_conf.h  | 2 +-
>  src/qemu/qemu_command.c | 2 +-
>  src/vmx/vmx.c           | 3 +--
>  4 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 402e6e9..d4c1054 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2655,7 +2655,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def)
>  
>      for (i = 0; i < def->ncontrollers; i++) {
>          cont = def->controllers[i];
> -        if (cont->idx > max_idx[cont->type])
> +        if ((int)cont->idx > max_idx[cont->type])
                    ^ missing space

>              max_idx[cont->type] = cont->idx;
>      }
>  
> @@ -2663,7 +2663,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def)
>      max_idx[VIR_DOMAIN_CONTROLLER_TYPE_USB] = -1;
>  
>      for (i = 0; i < VIR_DOMAIN_CONTROLLER_TYPE_LAST; i++) {
> -        if (max_idx[i] >= 0 && !(bitmaps[i] = virBitmapNew(max_idx[i] + 1)))
> +        if (max_idx[i] >= 0 && !(bitmaps[i] = virBitmapNew((size_t)max_idx[i] + 1)))

This cast seems unnecessary.

>              goto no_memory;
>          nbitmaps++;
>      }
> @@ -5593,7 +5593,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>  
>      idx = virXMLPropString(node, "index");
>      if (idx) {
> -        if (virStrToLong_i(idx, NULL, 10, &def->idx) < 0) {
> +        if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 || def->idx > INT_MAX) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Cannot parse controller index %s"), idx);
>              goto error;

If we parse it as unsigned, we should print it as unsigned too. I've split the
line over 80 columns and squashed this in:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b515887..354131e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14382,7 +14382,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
     }

     virBufferAsprintf(buf,
-                      "    <controller type='%s' index='%d'",
+                      "    <controller type='%s' index='%u'",
                       type, def->idx);

     if (model) {


> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index da83eb6..7897b4b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -790,7 +790,7 @@ struct _virDomainVirtioSerialOpts {
>  /* Stores the virtual disk controller configuration */
>  struct _virDomainControllerDef {
>      int type;
> -    int idx;
> +    unsigned int idx;
>      int model; /* -1 == undef */
>      unsigned int queues;
>      union {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 46db28a..7fd1cbf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1585,7 +1585,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>  
>          for (i = 0; i < def->ncontrollers; i++) {
>              if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> -                if (def->controllers[i]->idx > max_idx)
> +                if ((int)def->controllers[i]->idx > max_idx)
                            ^ missing space
>                      max_idx = def->controllers[i]->idx;
>              }
>          }
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 5464d13..deddfaa 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1664,8 +1664,7 @@ virVMXParseConfig(virVMXContext *ctx,
>  
>      for (controller = 0; controller < def->ncontrollers; ++controller) {
>          if (def->controllers[controller]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> -            if (def->controllers[controller]->idx < 0 ||
> -                def->controllers[controller]->idx > 3) {
> +            if (def->controllers[controller]->idx > 3) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("SCSI controller index %d out of [0..3] range"),
>                                 def->controllers[controller]->idx);
> 

ACK and pushed with a test added:

diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml
b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml
new file mode 100644
index 0000000..be3d8f2
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-negative-index-invalid.xml
@@ -0,0 +1,15 @@
+<domain type='qemu'>
+  <name>fdr-br</name>
+  <memory unit='KiB'>2097152</memory>
+  <currentMemory unit='KiB'>2097152</currentMemory>
+  <vcpu placement='static' cpuset='0-1'>2</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-1.2'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <devices>
+    <emulator>/usr/libexec/qemu-kvm</emulator>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='pci' index='-1' model='pci-bridge'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d0d9cad..7d7332f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1015,6 +1015,9 @@ mymain(void)
     DO_TEST("mlock-off", QEMU_CAPS_MLOCK);
     DO_TEST("mlock-unsupported", NONE);

+    DO_TEST_PARSE_ERROR("pci-bridge-negative-index-invalid",
+                        QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
+
     virObjectUnref(driver.config);
     virObjectUnref(driver.caps);
     virObjectUnref(driver.xmlopt);

Jan

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list