On Tue, Feb 20, 2018 at 10:38:41AM +0100, Ján Tomko wrote:
> On Thu, Feb 15, 2018 at 04:43:06PM +0000, Daniel P. Berrangé wrote:
> > The controller model is slightly unusual in that the default value is
> > -1, not 0. As a result the default value is not covered by any of the
> > existing enum cases. This in turn means that any switch() statements
> > that think they have covered all cases, will in fact not match the
> > default value at all. In the qemuDomainDeviceCalculatePCIConnectFlags()
> > method this has caused a serious mistake where we fallthough from the
>
> s/fallthough/fallthrough/
>
> > SCSI controller case, to the VirtioSerial controller case, and from
> > the USB controller case to the IDE controller case.
> >
> > By adding explicit enum constant starting at -1, we can ensure switches
> > remember to handle the default case.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/conf/domain_addr.c | 5 +++++
> > src/conf/domain_conf.c | 1 +
> > src/conf/domain_conf.h | 4 ++++
> > src/qemu/qemu_command.c | 17 ++++++++++++++---
> > src/qemu/qemu_domain.c | 10 +++++++++-
> > src/qemu/qemu_domain_address.c | 22 ++++++++++++++++++++++
> > src/vbox/vbox_common.c | 13 +++++++++----
> > 7 files changed, 64 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> > index 6422682391..df19c6be1a 100644
> > --- a/src/conf/domain_addr.c
> > +++ b/src/conf/domain_addr.c
> > @@ -39,6 +39,7 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
> > * the equivalent VIR_PCI_CONNECT_TYPE_*.
> > */
> > switch (model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> > @@ -344,6 +345,9 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
> > bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST;
> > break;
> >
> > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> > + break;
> > +
>
> Unlike '-usb' for USB controllers, there is no -pci for pci controllers
> - we need to know the model at the time of building the command line.
>
> Not having one set here is either an error in the user input or the
> part of our code that should have filled the model in. If we want to be
> robust, this should be grouped with the next case.
Yes, I've checked test and latter it fallthrough to _LAST works too
>
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > "%s", _("PCI controller model was not set correctly"));
> > @@ -1746,6 +1750,7 @@ virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> > return cont->opts.usbopts.ports;
> > return 8;
> >
> > + case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> > break;
>
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 6c73cd7bfe..2a75a169c2 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 178ec24ae7..e114f5dfcf 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4098,11 +4098,16 @@ qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps,
> > case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> > case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
> > case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> > - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("Unsupported controller model: %s"),
> > virDomainControllerModelSCSITypeToString(model));
> > return false;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Unexpected SCSI controller model %d"),
> > + model);
> > + return false;
> > }
> >
> > return true;
>
> [...]
>
> > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> > index e28119e4b1..de565dbf74 100644
> > --- a/src/qemu/qemu_domain_address.c
> > +++ b/src/qemu/qemu_domain_address.c
> > @@ -513,6 +513,16 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> >
> > case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > switch ((virDomainControllerModelUSB) cont->model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> > + /* XXX it isn't clear that this is the right
> > + * thing to do here. We probably ought to
> > + * return 0, but that breaks test suite right
> > + * now because we call this method before we
> > + * have turned the _DEFAULT case into a
> > + * specific choice
> > + */
>
> qemuDomainControllerDefPostParse should have filled this one in for
> most QEMU machine/capabilities combinations.
> If it's still at default at this point, we're going to use "-usb",
> which should be a PCI device.
Ok, that's a useful explanation. I'll use that as the comment.
>
> > + return pciFlags;
> > +
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
> > return pcieFlags;
> > @@ -540,6 +550,16 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> >
> > case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > switch ((virDomainControllerModelSCSI) cont->model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
> > + /* XXX it isn't clear that this is the right
> > + * thing to do here. We probably ought to
> > + * return 0, but that breaks test suite right
> > + * now because we call this method before we
> > + * have turned the _DEFAULT case into a
> > + * specific choice
> > + */
> > + return pciFlags;
> > +
>
> Slightly different: a default SCSI controller cannot be formatted unless
> we turn it into a model. qemuDomainSetSCSIControllerModel in qemuDomainControllerDefPostParse
> errors out if we have no QEMU capabilities, (which probably should be
> relaxed as it might break loading of existing domains if the QEMU binary
> is not present). But the test suite passes even with return 0, so the
> comment is misleading.
Yes, my bad I'll remove that.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
