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...
> + 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.
(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.)
> + 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 :-)
> + break;
> }
>
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> @@ -1170,8 +1176,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0)
> goto error;
> releaseaddr = true;
> - if ((hostdev->source.subsys.u.pci.backend
> - != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) &&
> + if ((*backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) {
> configfd = qemuOpenPCIConfig(hostdev);
> if (configfd >= 0) {
Other than my dislike of typecasts and unreachable switch cases (which
may also be fine, but I want to see someone else say so too), this looks
fine, so ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list