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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/22/2015 03:54 PM, John Ferlan wrote:
>
> 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 ;-)


Yes, the end of the discussion is that <model type='blah'/> <target
blah='blurg' .../> is the least ugly of the available options.


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

But that would set options->type to NULL in the case that deviceName
hadn't been set, and we don't want that. Consider for a moment that
options->type had already been explicitly set to "pci-bridge". Since
options->type != NULL, deviceName would remain NULL, we would get to the
VIR_STRDUP() and unconditionally set options->type to NULL, losing the
value that we actually wanted (and leaking it to boot).

Hmm. I guess my logic *is* a bit backwards though :-) Instead I should
say "if (!options->type && VIR_STRDUP(blah)...)"

>
>>                  /* 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' ?

Yes, that sounds better.

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

I kind of like having the "autogenerated ..." error message because it
makes it clearer that libvirt was supposed to add something and didn't.

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]