On Tue, Feb 04, 2014 at 08:37:28AM +0100, Michal Privoznik wrote:
On 04.02.2014 08:05, Martin Kletzander wrote:
On Mon, Feb 03, 2014 at 10:12:58AM -0700, Eric Blake wrote:
On 02/03/2014 09:16 AM, Michal Privoznik wrote:
So far, we have just bare virDomainSuspend() API that suspends a domain. However, in the future there might occur a case, in which we may want to modify suspend behavior slightly. In that case, @flags are useful.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 2 ++ src/driver.h | 5 +++++ src/libvirt.c | 49 ++++++++++++++++++++++++++++++++++++++++---- src/libvirt_public.syms | 5 +++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 +++++++++++- src/remote_protocol-structs | 5 +++++ 7 files changed, 75 insertions(+), 5 deletions(-)
Back when we added virDomainShutdownFlags in 0.9.10, I asked if we should also add *Flags variants for remaining functions without them; at the time, we decided against it, but I'm not quite sure why.
I'm perfectly fine with adding this for the sake of making future additions easier, even if we don't have a use for the flags now - it's easier to support a flag than it is to rebase to pick up a new function for any situation where the .so contains a flags function, but it may be worth getting a second opinion before pushing, if you don't have a plan to use flags right away.
I like this approach as there are many issues that can be easily solved in case there is a 'Flags' version of some API. That's why we advocate usage of a flags parameter in new APIs even when it is not yet used.
Although I was wondering whether it would be too much overkill to use 'Params' instead of 'Flags' as Jiri did with migrations as that has way more power. And that's for both new APIs and this change proposed by Michal.
I think migration API is different to these ones. I mean, with migration you want to pass tons of arguments (from spoofing domain XML, through changing domain name on the dst or graphic listen address, to limiting migration bandwidth). However, with suspend or resume it's different. We haven't been missing even flags till now, nor Typed Params.
For instance, one usage scenario (and this answers Dan's question): At the resume, when domain's CPUs were not running for a certain period of time (and guest basically doesn't know nothing about it), users might want to resync the guest time. There's currently one approach being discussed on the qemu-devel: The qemu-ga has this guest-set-time which requires a time to be passed (currently). With the approach, the time argument becomes optional, so that whenever it is not passed with the command, the qemu-ga reads current time from guest's RTC. So in libvirt we could then just:
virDomainResume(dom, VIR_DOMAIN_RESUME_SYNC_TIME);
or something. And for virDomainSupsend I don't have any particular example, but since suspend and resume are a pair, I've changed both of them.
So there's interesting, and I'm not sure I entirely agree about adding flags here. It seems to me that if the QEMU agent has a "set-time" capability we'd be better off having an explicit API virDomainSetTime(...). The action of resume + set time cannot be atomic so I don't see any point in overloading the "set time" functionality into the resume API call. Just let apps call the set time method if they so desire.
tl;dr - TypedParams are bit overkill IMO.
Agreed, TypedParams are also pretty nasty to work with as an application developer, so we should only use them where absolutely required. 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 :|