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