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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list