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

Re: [libvirt] [PATCH 01/15] Introduce virDomainSuspendFlags

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

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