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@xxxxxxxxxx> > >>>--- > >>> 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list