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

[Anthony Liguori]

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