[libvirt] [PATCH] qemu: avoid double reservation of PCI address when hotplugging interface type='hostdev'

[Andrea Bolognani]

On Sat, 2019-10-19 at 02:18 -0400, Laine Stump wrote:
> Commit 01ca4010d86 (libvirt v5.1.0) moved address reservation for
> hotplugged interface devices up to an earlier point in
> qemuDomainAttachNetDevice() because some function called when
> hotplugging on aarch64 needed to know the address type, and failed
> when it was "none".

I don't see anything in the original commit suggesting the change
was connected to aarch64? In fact, for the most part I would expect
aarch64/virt guests to go down the very same code paths as x86/q35
guests.

> This unfortunately caused a regression, because it also made PCI
> address reservation happen before we noticed that the device was a
> *hostdev* interface. Those interfaces are hotplugged by just calling
> out to qemuDomainAttachHostdevDevice() - that function would then also
> attempt to reserve the *same PCI address* that had just been reserved
> in qemuDomainAttachNetDevice().

And of course at the time we didn't have, and after this patch still
don't have, test suite coverage for that O:-)

> The solution is to move the bit of code that short-circuits out to
> virDomainHostdevAttach() up *even earlier* so that no PCI address has
> been allocated by the time it's called.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1744523
> Signed-off-by: Laine Stump <laine at redhat.com>

The prevailing style seems to be

  ...
  been allocated by the time it's called.

  https://bugzilla.redhat.com/show_bug.cgi?id=1744523

  Signed-off-by: Laine Stump <laine at redhat.com>

Feel free to change it or leave it as-is, I personally don't feel
very strongly either way.

> +++ b/src/qemu/qemu_hotplug.c
> +    case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> +        /* this switch is skipped by hostdev interfaces */

Maybe something like

  /* hostdev interfaces have been dealt with earlier */

instead?


With at least the reference to aarch64 removed (unless of course I
have misunderstood the situation and the original commit was really
aarch64-related),

  Reviewed-by: Andrea Bolognani <abologna at redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization