[libvirt] [PATCH] qemu: forbid user aliases for implicit pci-root controllers

[Ján Tomko]

On Wed, Nov 29, 2017 at 04:27:16PM +0100, Michal Privoznik wrote:
>On 11/29/2017 03:15 PM, Jiri Denemark wrote:
>> On Wed, Nov 29, 2017 at 11:57:30 +0100, Michal Privoznik wrote:
>>> On 11/28/2017 02:51 PM, Ján Tomko wrote:
>>>> For implicit controllers, we do not format user aliases on the command
>>>> line so we provide a translation between the user alias and the qemu
>>>> alias.
>>>>
>>>> However for pci-root controllers, the logic determining whether to use
>>>> 'pci' or 'pci.0' depends on:
>>>> * domain architecture
>>>> * machine type
>>>> * QEMU version (for PPC)
>>>>
>>>> Since we do not store the QEMU version in status XML and as of
>>>> commit 5b783379 we no longer have the QEMU_CAPS_PCI_MULTIBUS
>>>> capability formatted there either, we can only rely on the alias
>>>> of the controller formatted there.
>>>>
>>>> Forbid user aliases for the implicit pci-root controllers so that
>>>> we retain this information.
>>>>
>>>> This allows us to drop the call to virQEMUCapsHasPCIMultiBus,
>>>> and fixing hotplug after daemon restart, since virQEMUCapsHasPCIMultiBus
>>>> relies on qemuCaps->arch which is not filled out by reading
>>>> the status XML either.
>>>>
>>>> Partially reverts commit 937f3195 which added the user alias ->
>>>> qemu alias mapping for implicit PCI controllers.
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1518148
>>>> ---
>>>>
>>>> While we cannot reliably map user aliases to qemu aliases in
>>>> every corner case, I am not sure where to draw the line.
>>>>
>>>> For x86_64, HasPCIMultiBus could be fixed by using def->os.arch instead
>>>> of qemuCaps->arch. So only PPC is unfixable, but this looked like the option
>>>> with the least amount of exceptions.
>>>>
>>>>  src/conf/domain_conf.c                             |  3 +--
>>>>  src/conf/domain_conf.h                             |  2 ++
>>>>  src/qemu/qemu_command.c                            | 16 +++++--------
>>>>  src/qemu/qemu_domain.c                             | 26 +++++++++++++++++++++-
>>>>  .../qemuxml2argvdata/qemuxml2argv-user-aliases.xml |  4 +---
>>>>  5 files changed, 35 insertions(+), 16 deletions(-)
>>>
>>> I'm not a fan of this. If virQEMUCapsHasPCIMultiBus() returns different
>>> results before and after libvirtd is restarted than it will bite us
>>> again in the future. While you're fixing one instance of the bug, the
>>> bug itself is still present. I suggest we extend our status XML to
>>> contain all the info needed so that we can make right decision
>>> regardless of how many times libvirtd is restarted.
>>
>> Which would break any running domains, because they won't have the right
>> info there...
>
>Well, there's no good solution here that'd work 100%, is there. What we
>might do though is to forbid setting user aliases on those ancient qemu
>which lack the multibus capability.
>

Either way, commit 937f3195 needs to be completely or partially reverted
before the release, since it breaks hotplug for domains that don't even
use aliases.

I do not particularly care whether the partial revert will be
accompanied by breaking user aliases for implicit pci-root (as proposed
here) or breaking user aliases for old QEMUs where HasPCIMultiBus
is unfixable (proposed in: [PATCH 0/4] Fix hotplug after daemon restart)
https://www.redhat.com/archives/libvir-list/2017-November/msg01151.html

Jan

>Michal
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171130/64824ea8/attachment-0001.sig>