[libvirt] [PATCH] add checking index for pci-bridge controller

[Laine Stump]

On 07/04/2013 07:10 AM, Jincheng Miao wrote:
> hi all,
>
> There is a bug here https://bugzilla.redhat.com/show_bug.cgi?id=981261 about controller configuration.
> For the pci-bridge controller, if index less than zero, there is no alert for it, and libvirt will pass this 
> value to qemu-kvm. But qemu-kvm will report the argument error, like:
> "qemu-kvm: -device pci-bridge,chassis_nr=-1,id=pci.-1,bus=pci.0,addr=0x9: Parameter 'chassis_nr' expects uint8_t"
>
> So libvirt should check the arguments of controller.
>
> Here is my patch for pci-bridge：
>
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5693,6 +5693,14 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>                                   "have an address"));
>                  goto error;
>              }
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +            if (def->idx < 0) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("pci-bridge controller index invalid, should not "
> +                                 "less than zero"));
> +                goto error;
> +            }
> +

1) When sending a patch, please use git send-email, so that the patch is
in a form that can be directly applied to a local tree with "git am".

2) That seems like a reasonable check for the index of *any* controller,
not just pci-bridge. You could either put in a check right after the
call to virStrToLong_i(), or alternately call virStrToLong_ui() instead
(you would have to do it into a temporary variable, or change the
definition of idx to be unsigned int (will size_t work here?). I'm not
sure if there's a specific reason why idx was declared signed instead,
as I haven't examined the code in detail, but it's very likely we could
change it with no ill effect.

>          }
>
> And I also see qemuBuildControllerDevStr() in qemu_command.c 
>         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>             if (def->idx == 0) {
>                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                _("PCI bridge index should be > 0"));
>                 goto error;
>             }
>             virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d",
>                               def->idx, def->idx);
>
> And if I modify it to "def->idx > 0", testsuite will fail.
> So why only check "def->idx == 0" ?  


This is specifically checking for index 0, because index 0 is reserved
for the implicit "pci-root" controller (soon to be "pcie-root" on q35
machine types) that is built into the virtual machine.