Re: [libvirt] [PATCHv2 06/17] qemu: implement <model> subelement to <controller>

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; >