Re: [PATCHv3 1/3] qemu: hostdev: Refactor PCI passhrough handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]