On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:
On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote: [...]
Worse, a machine type property that is static for all machine types now could conceivably become dynamic when we add a machine type configuration knob.
This isn't the first time a machine capability that seems static actually depends on other configuration arguments. We will probably need to address this eventually.
Then the best time to address it is now, provided we can :)
I'm not sure this is the best time. If libvirt only needs the runtime value and don't need any info at query-machines time, I think support for this on query-machines will be left unused and they will only use the query-current-machine value.
Just giving libvirt the runtime data it wants (query-current-machine) seems way better than requiring libvirt to interpret a set of rules and independently calculate something QEMU already knows.
I wouldn't mind adding a query-current-machine to report dynamic machine capabilities if that helps QMP clients. query-machines could continue to report static machine capabilities then.
However, we do need a plan on how to distribute machine capabilities between query-machines and query-current-machine, in particular how to handle changing staticness.
Handling dynamic data that becomes static is easy: we can keep it on both.
wakeup-suspend-support is static for most machine types, but dynamic for some. Where should it go?
I think it obviously should go on query-current-machine. Maybe it can be added to query-machines in the future if it's deemed useful.
It needs to go into query-current-machine when its dynamic with the current machine. It may go there just to keep things regular even if its static with the current machine.
Does it go into query-machines, too? If not, clients lose the ability to examine all machines efficiently. Even if this isn't an issue for wakeup-suspend-support: are we sure this can't be an issue for any future capabilities?
I don't think this will be an issue for wakup-suspend-support, but this is already an issue for existing capabilities. See below[1].
If it goes into query-machines, what should its value be for the machine types where it's dynamic? Should it be absent, perhaps, letting clients know they have to consult query-current-machine to find the value?
What if a capability previously thought static becomes dynamic? We can add it to query-current-machine just fine, but removing it from query-machines would be a compatibility break. Making it optional, too. Should all new members of MachineInfo be optional, just in case?
The above are questions we must ponder if we are considering extending query-machines. We have been avoiding them for a few years, by simply not extending query-machines very often and letting libvirt hardcode machine-type info. :(
These are design questions we need to ponder *now*. Picking a solution that satisfies current needs while ignoring future needs has bitten us in the posterior time and again. We're not going to successfully predict *all* future needs, but not even trying should be easy to beat.
That's what I meant by "the best time to address it is now".
I agree. I just think there are countless other use cases that require extending query-machines today. I'd prefer to use one of them as a starting point for this design exercise, instead of wakeup-suspend-support. [1] Doing a: $ git grep 'STR.*machine, "' on libvirt source is enough to find some code demonstrating where query-machines is already lacking today: src/libxl/libxl_capabilities.c:583: if (STREQ(machine, "xenpv")) src/libxl/libxl_capabilities.c:729: if (STREQ(domCaps->machine, "xenfv")) src/libxl/libxl_driver.c:6379: if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { src/qemu/qemu_capabilities.c:2435: STREQ(def->os.machine, "ppce500")) src/qemu/qemu_capabilities.c:2439: STREQ(def->os.machine, "prep")) src/qemu/qemu_capabilities.c:2443: STREQ(def->os.machine, "bamboo")) src/qemu/qemu_capabilities.c:2446: if (STREQ(def->os.machine, "mpc8544ds")) src/qemu/qemu_capabilities.c:5376: STREQ(def->os.machine, "isapc"); src/qemu/qemu_command.c:3829: if (STRPREFIX(def->os.machine, "s390-virtio") && src/qemu/qemu_domain.c:2798: if (STREQ(def->os.machine, "isapc")) { src/qemu/qemu_domain.c:5009: if (STREQ(def->os.machine, "versatilepb")) src/qemu/qemu_domain.c:8222: return (STRPREFIX(machine, "pc-q35") || src/qemu/qemu_domain.c:8223: STREQ(machine, "q35")); src/qemu/qemu_domain.c:8237: return (STREQ(machine, "pc") || src/qemu/qemu_domain.c:8238: STRPREFIX(machine, "pc-0.") || src/qemu/qemu_domain.c:8239: STRPREFIX(machine, "pc-1.") || src/qemu/qemu_domain.c:8240: STRPREFIX(machine, "pc-i440") || src/qemu/qemu_domain.c:8241: STRPREFIX(machine, "rhel")); src/qemu/qemu_domain.c:8285: const char *p = STRSKIP(machine, "pc-q35-"); src/qemu/qemu_domain.c:8310: return STRPREFIX(machine, "s390-ccw"); src/qemu/qemu_domain.c:8329: if (STRNEQ(machine, "virt") && src/qemu/qemu_domain.c:8330: !STRPREFIX(machine, "virt-")) src/qemu/qemu_domain.c:8351: if (STRNEQ(machine, "pseries") && src/qemu/qemu_domain.c:8352: !STRPREFIX(machine, "pseries-")) src/qemu/qemu_domain.c:8564: STREQ(machine, "malta") || src/qemu/qemu_domain.c:8565: STREQ(machine, "sun4u") || src/qemu/qemu_domain.c:8566: STREQ(machine, "g3beige"); src/qemu/qemu_domain_address.c:467: if (!(STRPREFIX(def->os.machine, "vexpress-") || src/qemu/qemu_domain_address.c:2145: if (STREQ(def->os.machine, "versatilepb")) src/util/virarch.c:170: } else if (STREQ(ut.machine, "amd64")) { -- Eduardo