9:16 a.m.
On 10/07/13 15:57, Laine Stump wrote:
On 10/04/2013 08:55 AM, Peter Krempa wrote:
> To simplify future patches dealing with this code, simplify and refactor
> some conditions to switch statements.
> ---
> src/qemu/qemu_command.c | 27 ++++++++++++++++++---------
> src/qemu/qemu_hotplug.c | 27 ++++++++++++++++-----------
> 2 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e976466..ecf26cc 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5485,14 +5485,25 @@ qemuBuildPCIHostdevDevStr(virDomainDefPtr def,
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> - if (dev->source.subsys.u.pci.backend
> - == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> - virBufferAddLit(&buf, "vfio-pci");
> - } else {
> + switch ((virDomainHostdevSubsysPciBackendType)
> + dev->source.subsys.u.pci.backend) {
I'm assuming you're doing the typecast so that you're forced to have a
case for every enum value. Do we have an official policy on that? I see
similar typecasts for switch statements in a few places, but I don't
know that I'm so fond of it...
I started adding this on multiple places earlier. I don't know if
there's a official strategy, but I tend to prefer it as I was bitten a
few times by adding a new enum value and missed a few switches where I
actually should handle those. That's why I'm adding the typecasts where
possible.
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> virBufferAddLit(&buf, "pci-assign");
> if (configfd && *configfd)
> virBufferAsprintf(&buf, ",configfd=%s", configfd);
> + break;
> +
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> + virBufferAddLit(&buf, "vfio-pci");
> + break;
> +
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("PCI passhthrough type needs to be specified"));
And this is one reason why - this value will *never* happen. If nothing
is specified, backend will be set to "...DEFAULT", not
"...TYPE_LAST".
As a matter of fact, by the time you get as low as this function,
backend will always be default, kvm, or vfio - the higher level
functions see to that. There is a point where double checking values
down through all the levels of a call chain becomes pointless, and I
think we've reached it here.
Yes, the _TYPE_LAST will never happen here. I usually leave out any
action on the _TYPE_LAST variants, but in this particular case, patch
3/3 changes the default and code in this function shouldn't ever be
reached with _DEFAULT any more. danpb suggested adding this error.
(Personally, I think if we're going to do enforce explicit listing of
all cases in switch statements for an attribute that has an enum
associated with it, it would be better to define the field with the
actual enum type rather than int - these are internal data structures
that are never passed from one machine to another, so it's not like
there would ever be any compatibility problem.)
That would be a ideal global solution. Unfortunately as the
SOMETypeFromString macroed functions return -1 on failure almost all
variables holding the value got by this functions are declared as int
rather than the appropriate enum value. To avoid the need for a separate
intermediate variable we tend to declare the holding vars as int.
> + break;
> }
> +
> virBufferAsprintf(&buf, ",host=%.2x:%.2x.%.1x",
> dev->source.subsys.u.pci.addr.bus,
> dev->source.subsys.u.pci.addr.slot,
> @@ -9232,7 +9243,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> VIR_FREE(devstr);
> }
>
> -
> /* Add host passthrough hardware */
> for (i = 0; i < def->nhostdevs; i++) {
> virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> @@ -9305,9 +9315,9 @@ qemuBuildCommandLine(virConnectPtr conn,
> /* PCI */
> if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> + int backend = hostdev->source.subsys.u.pci.backend;
>
> - if (hostdev->source.subsys.u.pci.backend
> - == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> + if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("VFIO PCI device assignment is not "
> @@ -9321,8 +9331,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> char *configfd_name = NULL;
> - if ((hostdev->source.subsys.u.pci.backend
> - != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) &&
> + if ((backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) {
> int configfd = qemuOpenPCIConfig(hostdev);
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 818c726..ae2cbc0 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1134,6 +1134,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> int configfd = -1;
> char *configfd_name = NULL;
> bool releaseaddr = false;
> + int *backend = &hostdev->source.subsys.u.pci.backend;
>
> if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) <
0)
> return -1;
> @@ -1142,10 +1143,8 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> &hostdev, 1) < 0)
> return -1;
>
> - if (hostdev->source.subsys.u.pci.backend
> - == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> - unsigned long long memKB;
> -
> + switch ((virDomainHostdevSubsysPciBackendType) *backend) {
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("VFIO PCI device assignment is not "
> @@ -1157,11 +1156,18 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> * In this case, the guest's memory may already be locked, but it
> * doesn't hurt to "change" the limit to the same value.
> */
> - vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> - memKB = vm->def->mem.hard_limit ?
> - vm->def->mem.hard_limit : vm->def->mem.max_balloon + 1024 *
1024;
> - virProcessSetMaxMemLock(vm->pid, memKB);
> - vm->def->hostdevs[vm->def->nhostdevs--] = NULL;
> + if (vm->def->mem.hard_limit)
> + virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit);
> + else
> + virProcessSetMaxMemLock(vm->pid,
> + vm->def->mem.max_balloon + (1024 *
1024));
> +
> + break;
> +
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
Again - I dislike having these sentinel values showing up in switch
statements (searching the source I see it a lot), as it could lead
someone to believe that those are actually valid values. I may be
outvoted on this though :-)
I think that the benefits of the compiler warning in case you add a new
variable on places you might have forgotten outweigh a few occurences of
unused type in the code.
...
Peter