Eduardo Habkost <ehabkost@xxxxxxxxxx> writes: > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote: >> >> >> On 05/21/2018 03:14 PM, Eduardo Habkost wrote: >> > > Issue#2: the flag isn't a property of the target. Due to -no-acpi, it's >> > > not even a property of the machine type. If it was, query-machines >> > > would be the natural owner of the flag. >> > > >> > > Perhaps query-machines is still the proper owner. The value of >> > > wakeup-suspend-support would have to depend on -no-acpi for the machine >> > > types that honor it. Not ideal; I'd prefer MachineInfo to be static. >> > > Tolerable? I guess that's also a libvirt question. >> > It depends when libvirt is going to query it. Is it OK to only >> > query it after the VM is already up and running? If it is, then >> > we can simply expose it as a read-only property of the machine >> > object. >> > >> > Or, if we don't want to rely on qom-get as a stable API, we can >> > add a new query command (query-machine? query-power-management?) >> > >> In the first version this logic was included in a new query command called >> "query-wakeup-from-suspend-support": >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00889.html >> >> In that review it was suggested that this logic could be a flag in either >> query-target >> or query-machines API. Before sending the v2 I sent the following comment: >> >> "After investigating, I think that it's simpler to hook the wakeup support >> info into >> TargetInfo than MachineInfo, given that the detection I'm using for this new >> property >> is based on the current runtime state. Hooking into MachineInfo would >> require to >> change the MachineClass to add a new property, then setting it up for the >> machines >> that have the wakeup support (only x86 so far). Definitely doable, but if we >> don't >> have any favorites between MachineInfo and TargetInfo I'd rather pick the >> simpler >> route. >> >> So, if no one objects, I'll rework this series by putting the logic inside >> query-target >> instead of a new API." > > Apologies for not noticing this series months ago. :( Seconded. Daniel, this (minor) mess is absolutely not your fault. >> Since no objection was made back then, this logic was put into query-target >> starting >> in v2. Still, I don't have any favorites though: query-target looks ok, >> query-machine >> looks ok and a new API looks ok too. It's all about what makes (more) sense >> in the >> management level, I think. > > I understand the original objection from Eric: having to add a > new command for every runtime flag we want to expose to the user > looks wrong to me. Agreed. > However, extending query-machines and query-target looks wrong > too, however. query-target looks wrong because this not a > property of the target. query-machines is wrong because this is > not a static property of the machine-type, but of the running > machine instance. Of the two, query-machines looks less wrong. Arguably, -no-acpi should not exist. It's an ad hoc flag that sneakily splits a few machine types into two variants, with and without ACPI. It's silently ignored for other machine types, even APCI-capable ones. If the machine type variants with and without ACPI were separate types, wakeup-suspend-support would be a static property of the machine type. However, "separate types" probably doesn't scale: I'm afraid we'd end up with an undesirable number of machine types. Avoiding that is exactly why we have machine types with configurable options. I suspect that's how ACPI should be configured (if at all). So, should we make -no-acpi sugar for a machine type parameter? And then deprecate -no-acpi for good measure? > Can we have a new query command that could be an obvious > container for simple machine capabilities that are not static? A > name like "query-machine" would be generic enough for that, I > guess. Having command names differ only in a single letter is awkward, but let's focus on things other than naming now, and use query-current-machine like a working title. query-machines is wrong because wakeup-suspend-support isn't static for some machine types. query-current-machine is also kind of wrong because wakeup-suspend-support *is* static for most machine types. 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. Would a way to tie the property to the configuration knob help? Something like wakeup-suspend-support taking values true (supported), false (not supported), and "acpi" (supported if machine type configuration knob "acpi" is switched on). > Markus, Eric, what do you think? Haven't made up my mind, yet :) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
