On 07/17/2015 02:43 PM, Laine Stump wrote:
> This patch provides qemu support for the contents of <model> in
> <controller> for the two existing PCI controller types that need it
> (i.e. the two controller types that are backed by a device that must
> be specified on the qemu commandline):
>
> 1) pci-bridge - sets <model> type attribute default as "pci-bridge"
>
> 2) dmi-to-pci-bridge - sets <model> type attribute default as
> "i82801b11-bridge".
>
> These both match current hardcoded practice.
>
> The defaults are set at the end of qemuDomainAssignPCIAddresses(), It
> can't be done earlier because some of the options that will be
> autogenerated need full PCI address info for the controller and
> because qemuDomainAssignPCIAddresses() might create extra controllers
> which would need default settings added. This is still prior to the
> XML being written to disk, though, so the autogenerated defaults are
> persistent.
>
> qemu capabilities bits aren't checked until the commandline is
> actually created (so the domain can possibly be defined on a host that
> doesn't yet have support for the give n device, or a host different
> from the one where it will eventually be run). At that time we compare
> the type strings to known qemu device names and check for the
> capabilities bit for that device.
> ---
> new in V2 (previously was a part of the patch to add pcie-root-port)
>
> src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 64 insertions(+), 6 deletions(-)
>
Without trying to follow all the discussion from v1 of the series, I am
assuming this methodology has been "agreed" upon... Probably should
have noted that in the previous patch ;-)
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f02f5..8868e18 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> virDomainControllerDefPtr cont = def->controllers[i];
> int idx = cont->idx;
> virDevicePCIAddressPtr addr;
> + virDomainPCIControllerOptsPtr options;
> + const char *deviceName = NULL;
>
> if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> continue;
>
> addr = &cont->info.addr.pci;
> + options = &cont->opts.pciopts;
> +
> + /* set defaults for any other auto-generated config
> + * options for this controller that haven't been
> + * specified in config.
> + */
> + switch ((virDomainControllerModelPCI)cont->model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> + if (!options->type)
> + deviceName = "pci-bridge";
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> + if (!options->type)
> + deviceName = "i82801b11-bridge";
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> + break;
> + }
> + if (deviceName &&
> + VIR_STRDUP(options->type, deviceName) < 0)
> + goto cleanup;
> +
As has been told to me before - virStrdup/VIR_STRDUP is "tolerant" of
deviceName == NULL returning 0 if it's NULL, so just:
if (VIR_STRDUP(options->type, deviceName) < 0)
goto cleanup;
is necessary
> /* check if every PCI bridge controller's ID is greater than
> * the bus it is placed onto
> */
> @@ -4614,17 +4640,49 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
> }
> switch (def->model) {
> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
> + if (!def->opts.pciopts.type) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("autogenerated pci-bridge options not set"));
> + goto error;
> + }
> + if (STREQ(def->opts.pciopts.type, "pci-bridge")) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("the pci-bridge controller "
> + "is not supported in this QEMU binary"));
> + goto error;
> + }
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown pci-bridge device '%s'"),
unknown pci-bridge option model '%s' ?
> + def->opts.pciopts.type);
> + goto error;
> + }
Alternatively
if (STREQ_NULLABLE() {
} else {
error message using NULLSTR(def->opts.pciopts.type)
I don't care either way - it's obvious
> + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
> + def->opts.pciopts.type,
> def->idx, def->info.alias);
> break;
> case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("The dmi-to-pci-bridge (i82801b11-bridge) "
> - "controller is not supported in this QEMU binary"));
> + if (!def->opts.pciopts.type) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("autogenerated dmi-to-pci-bridge options not set"));
> + goto error;
> + }
> + if (STREQ(def->opts.pciopts.type, "i82801b11-bridge")) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("the dmi-to-pci-bridge (i82801b11-bridge) "
> + "controller is not supported in this QEMU binary"));
> + goto error;
> + }
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown dmi-to-pci-bridge device '%s'"),
unknown dmi-to-pci-bridge option model '%s' ??
> + def->opts.pciopts.type);
> goto error;
> }
Same here - again - doesn't really matter
ACK with the VIR_STRDUP() cleanup - whether you adjust the error message
or the if/else conditions is your call.
John
> - virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias);
> + virBufferAsprintf(&buf, "%s,id=%s",
> + def->opts.pciopts.type, def->info.alias);
> break;
> }
> break;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list