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

[John Ferlan]

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