Re: [libvirt] [PATCH] add checking index for pci-bridge controller
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.