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.