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