[libvirt] [RFC PATCH] threshold: new API virDomainBlockSetWriteThreshold

[Daniel P. Berrange]

On Mon, May 25, 2015 at 11:11:14AM +0200, Peter Krempa wrote:
> On Sat, May 23, 2015 at 21:45:56 -0600, Eric Blake wrote:
> > qemu 2.3 added a new QMP command block-set-write-threshold,
> > which allows callers to get an interrupt when a file hits a
> > write threshold, rather than the current approach of repeatedly
> > polling for file allocation.  This patch prepares the API for
> > callers to register to receive the event, as well as a way
> > to query the threshold.
> > 
> > The event is one-shot in qemu - a guest must re-register a new
> > threshold after each time it triggers.  However, the
> > virConnectDomainEventRegisterAny() call does not allow
> > parameterization, so callers must use a pair of APIs - one
> > to register the callback (one-time call), and another to
> > repeatedly set the desired threshold (repeated each time a
> > threshold changes).
> > 
> > Note that the threshold is registered as a double, but reported
> > as an unsigned long long.  This is intentional, as it allows
> > the use of a flag for registering a threshold via a percentage,
> > but once registered, the threshold is normalized according to
> > the current size of the disk.
> > 
> > To make the patch series more digestible, this patch
> > intentionally omits remote support, by using a couple of
> > placeholders at a point where the compiler forces the addition
> > of a case within a switch statement.
> > 
> > Signed-off-by: Eric Blake <eblake at redhat.com>
> > ---
> > 
> > Posting now to get feedback on the proposed API, before the actual
> > implementation is complete.
> > 
> >  daemon/remote.c                  |   2 +
> >  include/libvirt/libvirt-domain.h |  48 +++++++++++++++++++
> >  src/conf/domain_event.c          |   4 +-
> >  src/driver-hypervisor.h          |   7 +++
> >  src/libvirt-domain.c             | 101 +++++++++++++++++++++++++++++++++++++++
> >  src/libvirt_public.syms          |   1 +
> >  tools/virsh-domain.c             |  23 +++++++++
> >  tools/virsh.pod                  |   1 +
> >  8 files changed, 186 insertions(+), 1 deletion(-)
> > 
> > diff --git a/daemon/remote.c b/daemon/remote.c
> > index e259a76..51d7de5 100644
> > --- a/daemon/remote.c
> > +++ b/daemon/remote.c
> > @@ -1102,6 +1102,8 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
> >      VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventTunable),
> >      VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventAgentLifecycle),
> >      VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceAdded),
> > +    /* TODO: Implement RPC support for this */
> > +    VIR_DOMAIN_EVENT_CALLBACK(NULL),
> >  };
> > 
> >  verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > index d851225..0c4fd62 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -515,6 +515,15 @@ typedef virDomainBlockStatsStruct *virDomainBlockStatsPtr;
> >  # define VIR_DOMAIN_BLOCK_STATS_ERRS "errs"
> > 
> >  /**
> > + * VIR_DOMAIN_BLOCK_STATS_WRITE_THRESHOLD:
> > + *
> > + * Macro represents the typed parameter, as an llong, that reports
> 
> Signed? The rest of the block stats fields is signed for a reason, is
> there a reason why it's here?
> 
> > + * the threshold in bytes at which the block device will trigger a
> > + * VIR_DOMAIN_EVENT_ID_WRITE_THRESHOLD, or 0 if no threshold is registered.
> 
> In case it remains signed, the negative values should be somehow
> described.
> 
> > + */
> > +# define VIR_DOMAIN_BLOCK_STATS_WRITE_THRESHOLD "write-threshold"
> > +
> > +/**
> >   * virDomainInterfaceStats:
> >   *
> >   * Network interface stats for virDomainInterfaceStats.
> > @@ -1297,6 +1306,16 @@ int                     virDomainBlockStatsFlags (virDomainPtr dom,
> >                                                    virTypedParameterPtr params,
> >                                                    int *nparams,
> >                                                    unsigned int flags);
> > +
> > +typedef enum {
> > +    /* threshold is a percentage rather than byte limit */
> > +    VIR_DOMAIN_BLOCK_SET_WRITE_THRESHOLD_PERCENTAGE = (1 << 0),
> > +} virDomainBlockSetWriteThresholdFlags;
> > +int virDomainBlockSetWriteThreshold(virDomainPtr dom,
> > +                                    const char *disk,
> > +                                    double threshold,
> 
> Double? Your last proposal used unsigned long long. Using double with
> byte sizes seems a bit odd to me. The file size won't exceed unsigned
> long long for a while so I don't see a reason to use double here.

Yes, I'm inclined to say u long long too, since that's what we use throughout
the APIs for representing disk image sizes. So in the unlikely event that
we do hit the limits of unsigned long long, we'd have countless APIs to
change regardless.

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