Re: [libvirt] [Qemu-devel] [PATCH RFC 0/4] Allow hibernation on guests
On 01/26/2012 09:18 AM, Eric Blake wrote:
[adding qemu-devel]
On 01/26/2012 07:46 AM, Daniel P. Berrange wrote:
>> One thing, that you'll probably notice is this
>> 'set-support-level' command. Basically, it tells GA what qemu version
>> is it running on. Ideally, this should be done as soon as
>> GA starts up. However, that cannot be determined from outside
>> world as GA doesn't emit any events yet.
>> Ideally^2 this command should be left out as it should be qemu
>> who tells its own agent this kind of information.
>> Anyway, I was going to call this command in qemuProcess{Startup,
>> Reconnect,Attach}, but it won't work. We need to un-pause guest CPUs
>> so guest can boot and start GA, but that implies returning from qemuProcess*.
>>
>> So I am setting this just before 'guest-suspend' command, as
>> there is one more thing about GA. It is unable to remember anything
>> upon its restart (GA process). Which has BTW show flaw
>> in our current code with FS freeze& thaw. If we freeze guest
>> FS, and somebody restart GA, the simple FS Thaw will not succeed as
>> GA thinks FS are not frozen. But that's a different cup of tea.
>>
>> Because of what written above, we need to call set-level
>> on every suspend.
>
>
> IMHO all this says that the 'set-level' command is a conceptually
> unfixably broken design& should be killed in QEMU before it turns
> into an even bigger mess.
>
> Once we're in a situation where we need to call 'set-level' prior
> to every single invocation, you might as well just allow the QEMU
> version number to be passed in directly as an arg to the command
> you are running directly thus avoiding this horrificness.
Qemu folks, would you care to chime in on this?
Big Ack on my part. I told Mike this afternoon that I wasn't going to take the
pull request with this command in it.
The fundamental problem here is simple--untested code is broken code. Until we
introduce a resume from suspend command (such that we can test the guest agent
suspend command), we shouldn't be implementing a guest-agent suspend command.
As far as I can tell, the only reason we're introducing it is because we're
trying to add a multiplexed command that does suspend to ram and suspend to
disk. Since it's multiplexed, it's an all-or-nothing introduction. We're
then
adding a side-interface to attempt to deal work around that.
Let's not introduce a multiplexed command in the first place. Here's what I
suggest:
1) Throw away set-level interface.
2) Introduce a suspend-to-disk command.
3) Plan to introduce a suspend-to-ram command, but I won't pull it until we have
the ability to test it successfully (which means we probably need a
resume-from-ram command for QMP).
4) libvirt can probe the existence of suspend-to-disk in the guest agent and act
accordingly.
5) To implement virDomainSuspendToRam (or whatever it will be called), libvirt
should:
a) check if the guest agent command 'suspend-to-ram' exists
b) check if the QMP command 'resume-from-ram' exists
6) The recommendation of (5) should be prominently documented in qapi-schema.json
7) In order for libvirt to start implementing (5), we should stub out (3) in
qapi-schema-guest.json but set gen=False. That commits us to the interface
without actually introducing the command.
Regards,
Anthony Liguori