[libvirt] [PATCHv3 02/13] qemu: implement <model> subelement to <controller>
Martin Kletzander
mkletzan at redhat.com
Mon Aug 3 10:05:38 UTC 2015
[I reduced the Cc list so I don't need to hear jtomko's whining again]
On Sat, Jul 25, 2015 at 03:58:26PM -0400, 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> name attribute default as "pci-bridge"
>
>2) dmi-to-pci-bridge - sets <model> name 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, and that hasn't been done at
>the time the PostParse callbacks are being
>run. qemuDomainAssignPCIAddresses() 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.
>---
>
>Changes from V2: use enum instead of string model name.
>
> src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 77 insertions(+), 4 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 09f30c4..6a19d15 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> virDomainControllerDefPtr cont = def->controllers[i];
> int idx = cont->idx;
> virDevicePCIAddressPtr addr;
>+ virDomainPCIControllerOptsPtr options;
>
> 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->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
>+ options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE;
>+ break;
>+ case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>+ if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
>+ options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_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;
>+ }
>+
> /* check if every PCI bridge controller's ID is greater than
> * the bus it is placed onto
> */
>@@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> int model = def->model;
>+ const char *modelName = NULL;
>
> if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0)
>@@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
> }
> switch (def->model) {
> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>- virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
>- def->idx, def->info.alias);
>+ if (def->opts.pciopts.modelName
>+ == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+ _("autogenerated pci-bridge options not set"));
>+ goto error;
>+ }
>+
>+ modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
>+ if (!modelName) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>+ _("unknown pci-bridge model name value %d"),
>+ def->opts.pciopts.modelName);
>+ goto error;
>+ }
>+ if (def->opts.pciopts.modelName
>+ != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+ _("PCI controller model name '%s' "
>+ "is not valid for a pci-bridge"),
>+ modelName);
>+ goto error;
>+ }
>+ 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;
>+ }
>+ virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
>+ modelName, def->idx, def->info.alias);
> break;
> case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>+ if (def->opts.pciopts.modelName
>+ == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+ _("autogenerated dmi-to-pci-bridge options not set"));
>+ goto error;
>+ }
>+
>+ modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
>+ if (!modelName) {
>+ virReportError(VIR_ERR_INTERNAL_ERROR,
>+ _("unknown dmi-to-pci-bridge model name value %d"),
>+ def->opts.pciopts.modelName);
>+ goto error;
>+ }
>+ if (def->opts.pciopts.modelName
>+ != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+ _("PCI controller model name '%s' "
>+ "is not valid for a dmi-to-pci-bridge"),
>+ modelName);
>+ goto error;
>+ }
I see why you didn't care whether it will be implemented this way or
the other. That's because you're checking for stuff we usually leave
the parsing to handle. I haven't checked that there is a possibility
of having domain parsed without assigning PCI addresses, but the
def->opts.pciopts.modelName cannot be invalid in this case. Anyway,
being _too_ careful cannot hurt, no need to change it unless you want
to.
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>- _("The dmi-to-pci-bridge (i82801b11-bridge) "
>+ _("the dmi-to-pci-bridge (i82801b11-bridge) "
> "controller is not supported in this QEMU binary"));
> goto error;
> }
You could also save a bunch of lines using what I suggested, that is
concentrating device->caps mapping in a separate function and using
one error message for all models, not one for every model, e.g.:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("the '%s' ('%s') controller is not supported "
"in this QEMU binary"),
blahTypeToString(def->model),
blehTypeToString(def->opts.pciopts.modelName));
but that can be done later on, the code works properly as it is now.
ACK
>- virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias);
>+ virBufferAsprintf(&buf, "%s,id=%s", modelName, def->info.alias);
> break;
> }
> break;
>--
>2.1.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150803/baf1b403/attachment-0001.sig>
More information about the libvir-list
mailing list