[libvirt] [PATCH 0/3] Support domain snapshots with current QMP whithout savevm command

[Daniel P. Berrange]

On Thu, Mar 03, 2011 at 03:04:58PM +0000, Daniel P. Berrange wrote:
> On Fri, Feb 25, 2011 at 07:04:07PM +0100, Jiri Denemark wrote:
> > This is quite hacky since it involves falling back to HMP when savevm
> > command is not found in QMP, which is something qemu monitor code was
> > not designed to support. Hence, I'm providing 2 version of the first
> > patch: 1.1/3 and 1.2/3.
> > 
> > - 1.1/3 version only touches JSON monitor code but involves copy&paste
> >   of the snapshot code from text monitor
> > 
> > - 1.2/3 touches more files but doesn't require duplicating the text
> >   monitor snapshot implementation into qemu_monitor_json.c. However, it
> >   results in somewhat funky call graphs:
> > 
> >   -> qemuMonitorJSONCreateSnapshot
> >   -> qemuMonitorTextCreateSnapshot
> >   -> qemuMonitorCommand (a macro expanding to qemuMonitorCommandWithFd)

Perhaps we could just add 'HMP' in the name here to indicate this
is just used for HMP based commands, and not the pure JSON ones.

> >   -> qemuMonitorJSONHumanCommandWithFd
> >   -> qemuMonitorJSONCommandWithFd
> > 
> >   The possibility to call qemuMonitorTextCreateSnapshot directly on JSON
> >   monitor is implemented by generalizing qemuMonitorCommandWithFd, which
> >   now either calls to qemuMonitorJSONHumanCommandWithFd or
> >   qemuMonitorTextCommandWithFd (former qemuMonitorCommandWithFd)
> >   depending on the monitor type.
> 
> This call path rather makes my head hurt but the lack of duplicated
> code is attractive.
> 
> > I prefer version 2 since it reuses text monitor implementation, but
> > other may prefer version 1, which is a bit more local...
> 
> I've suggested a possible 3rd alternative in my reply to v1

Ok, so having looked again at all 3 possible impls now, I'm going
to change my mind and agree with you that  1.2/3 is the likely
the best option due to the simplicity of adding HMP fallback to
arbitrary commands.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|