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

[Pavel Hrdina]

On 09/13/2014 12:12 AM, Eric Blake wrote:
> 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).

I've done the same thing as for remoteDeserializeTypedParamters. It 
wouldn't be only this one function so I'd rather leave it for separate 
patch series.

>
>> +++ 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?
>

No, there isn't documentation for all events and I would definitely 
document what could be stored in the typed parameters but in a patch
series for documentation for all events.

Using the macros would be fine except for vcpupin information as the 
name is "vcpu%d".

>> + *
>> + * 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.
>

Thanks for catching this bug, will fix it that the function will consume 
the params.

Pavel