Re: [Qemu-devel] [libvirt] [PATCH RFC 0/4] Allow hibernation on guests

[Anthony Liguori <anthony@codemonkey.ws>]

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