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

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(a)redhat.com&gt; > --- > +++ 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.