[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target |
Date: |
Fri, 25 May 2018 08:30:59 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Eduardo Habkost <address@hidden> writes:
> On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>>
>> > On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <address@hidden> writes:
>> >> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
>> > [...]
>> >> >> 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?
>> >
>> > I think we should.
>>
>> Would you like to take care of it?
>
> Adding to my TODO-list, I hope I will be able to do it before the
> next release.
Thanks!
>> >> > 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.
>> >
>> > The most appropriate solution depends a lot on how/when
>> > management software needs to query this.
>>
>> Right.
>>
>> > If they only need to query it at runtime for a running VM,
>> > there's no reason for us to go of our way and add complexity just
>> > to make it look like static data in query-machines.
>> >
>> > On the other hand, if they really need to query it before
>> > configuring/starting a VM, it won't be useful at all to make it
>> > available only at runtime.
>> >
>> > Daniel, when/how exactly software would need to query the new
>> > flag?
>> >
>> >
>> >> 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.
wakeup-suspend-support is static for most machine types, but dynamic for
some. Where should it go?
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?
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?
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".
[...]
- [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, (continued)
- [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/17
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/18
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/21
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Daniel Henrique Barboza, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/23
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/24
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/25
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Markus Armbruster, 2018/05/28
- Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target, Eduardo Habkost, 2018/05/29
[Qemu-devel] [PATCH v7 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions, Daniel Henrique Barboza, 2018/05/17
[Qemu-devel] [PATCH v7 3/3] qmp.c: system_wakeup: runstate and wake-up support check, Daniel Henrique Barboza, 2018/05/17