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

[Michal Privoznik]

On 11/30/2017 02:46 PM, Ján Tomko wrote:
> 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.

Okay. If you propose the patch I can review it.

Michal