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

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



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 redhat com
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


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