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

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



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)
>   -> 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

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 :|


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