On Fri, Aug 16, 2019 at 03:20:11PM +0200, Igor Mammedov wrote: > On Thu, 15 Aug 2019 15:38:03 -0300 > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > We have this issue reported when using libvirt to hotplug CPUs: > > https://bugzilla.redhat.com/show_bug.cgi?id=1741451 > > > > Basically, libvirt is not copying die-id from > > query-hotpluggable-cpus, but die-id is now mandatory. > > this should have been gated on compat property and affect > only new machine types. > Maybe we should do just that instead of fixup so libvirt > would finally make proper handling of query-hotpluggable-cpus. > > > > We could blame libvirt and say it is not following the documented > > interface, because we have this buried in the QAPI schema > > documentation: > > I wouldn't say buried, if I understand it right QAPI schema > should be the authoritative source of interface description. > > If I recall it's not the first time, there was similar issue > for exactly the same reason (libvirt not passing through > all properties from query-hotpluggable-cpus). > > And we had to fix it up on QEMU side (numa_cpu_pre_plug), > but it seems 2 years later libvirt is still broken the same way :( > > Should we really do fixups or finaly fix it on libvirt side?
Is it truly a bug in libvirt? Making QEMU behave differently when getting exactly the same input sounds like a bad idea, even if we documented that at the QAPI documentation. My suggestion is to instead drop the comment below from the QAPI documentation. New properties shouldn't become mandatory. > > > > Note: currently there are 5 properties that could be present > > > but management should be prepared to pass through other > > > properties with device_add command to allow for future > > > interface extension. This also requires the filed names to be kept in > > > sync with the properties passed to -device/device_add. > > > > But I don't think this would be reasonable from us. We can just > > make QEMU more flexible and let CPU properties to be omitted when > > there's no ambiguity. This will allow us to keep compatibility > > with existing libvirt versions. > > I don't really like making rule from exceptions so I'd suggest doing > it only for die_id if we have to do fix it up (with fat comment > like in numa_cpu_pre_plug). > The rest are working fine as is. I will insist we make it consistent for all properties, but I don't want this discussion to hold the bug fix. So I'll do this: I will submit a new patch that makes only die-id optional, and CC qemu-stable. After that, i will submit this patch again, and we can discuss whether we should make all properties optional. -- Eduardo
