[libvirt] [PATCH v2 3/5] event: introduce new event for cputune

[Eric Blake]

On 09/10/2014 06:08 AM, Pavel Hrdina wrote:
> This new event will use typedParameters to expose what has been actually
> updated and the reason is that we can in the future extend the cputune.
> With typedParameters we don't have to worry about creating some sort of
> v2 of cputune event if there will be new features implemented for cputune.

Nice - about time we planned for future extension in one of our events :)

> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---

> +++ b/daemon/remote.c
> @@ -111,6 +111,13 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val,
>                                   int *nparams);
>  
>  static int
> +remoteSerializeTypedParameters(virTypedParameterPtr params,
> +                               int nparams,
> +                               remote_typed_param **ret_params_val,
> +                               u_int *ret_params_len,
> +                               unsigned int flags);
> +

Hmm, is this static pre-declaration needed merely because the function
occurs after the new point at which you will be using it?  I'm a big fan
of topological sorting, and avoiding static declarations for
non-recursive functions (they are necessary for mutually-recursive
functions, but this is not a case of that).  If you'd like, a separate
patch that just does code motion to hoist the helper function to occur
earlier than any clients would be worthwhile - but not a showstopper for
this patch.  (If we do end up with a later patch series to reshuffle
things into topological order, it is more than just this function
impacted - and it's ideal to do such a reshuffle as a series where each
patch is easy to review, rather than trying to reshuffle lots of
functions at once).

> +++ b/include/libvirt/libvirt.h.in
> @@ -5153,6 +5153,24 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
>                                                             const char *devAlias,
>                                                             void *opaque);
>  
> +/**
> + * virConnectDomainEventCputuneCallback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @params: changed cpupin values stored as array of virTypedParameter
> + * @nparams: size of the array
> + * @opaque: application specified data
> + *
> + * This callback occurs when cpu tune is updated.

Are the expected typed parameter names, and expected associated types,
documented anywhere?  For many virTypedParameter functions, we have
macros giving the expected string name and doc comments for the
corresponding type; for the new bulk stats API, the expected names are
not constant but we are still doing a good job of documenting typical
name patterns and expected types.  Looking ahead, I see patch 5 adds at
least one call to virTypedParamsAddULLong("shares") - but the only place
I see "shares" mentioned in libvirt.h.in is VIR_DOMAIN_SCHEDULER_SHARES,
documented as an int rather than unsigned long long.  See my dilemma?

> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_CPUTUNE with virConnectDomainEventRegisterAny()
> + */
> +typedef void (*virConnectDomainEventCputuneCallback)(virConnectPtr conn,
> +                                                     virDomainPtr dom,
> +                                                     virTypedParameterPtr params,
> +                                                     int nparams,
> +                                                     void *opaque);

This looks good.

> +static virObjectEventPtr
> +virDomainEventCputuneNew(int id,
> +                         const char *name,
> +                         unsigned char *uuid,
> +                         virTypedParameterPtr params,
> +                         int nparams)
> +{
> +    virDomainEventCputunePtr ev;
> +
> +    if (virDomainEventsInitialize() < 0)
> +        return NULL;
> +
> +    if (!(ev = virDomainEventNew(virDomainEventCputuneClass,
> +                                 VIR_DOMAIN_EVENT_ID_CPUTUNE,
> +                                 id, name, uuid)))
> +        return NULL;
> +
> +    ev->params = params;
> +    ev->nparams = nparams;
> +

Awkward transfer semantics; caller must not free params after this
function succeeds.  But you don't consume params on error.  Looking
ahead at patch 5, I see a memory leak if this function fails, where the
caller qemuDomainPinVcpuFlags does:

+        event = virDomainEventCputuneNewFromDom(dom, eventParams,
eventNparams);

but fails to free eventParams if event was NULL (and because of the
transfer semantics, it must NOT free eventParams if event is non-NULL).
 I suppose I can live with transfer semantics (faster than deep-cloning
the params) - but you probably ought to document that it is intentional,
and for ease-of-use, I'd consider always freeing params here, even when
returning NULL.

Everything else looks okay.  So I'm just worried about documentation and
the potential for memory leaks on OOM.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140912/f25869c8/attachment-0001.sig>