[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]