Re: [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.
> 
> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
> ---

One more thing...


>  
> +/**
> + * 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.
> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_CPUTUNE with virConnectDomainEventRegisterAny()

It may be worth documenting that the callback must NOT free params or
its contents (that is, the callback handler must not use
virTypedEventParamsFree()).

>  static void
> +remoteDomainBuildEventCallbackCputune(virNetClientProgramPtr prog 
> ATTRIBUTE_UNUSED,
> +                                      virNetClientPtr client 
> ATTRIBUTE_UNUSED,
> +                                      void *evdata, void *opaque)
> +{
> +    virConnectPtr conn = opaque;
> +    remote_domain_event_callback_cputune_msg *msg = evdata;
> +    struct private_data *priv = conn->privateData;
> +    virDomainPtr dom;
> +    virTypedParameterPtr params = NULL;
> +    int nparams = 0;
> +    virObjectEventPtr event = NULL;
> +
> +    dom = get_nonnull_domain(conn, msg->dom);
> +    if (!dom)
> +        return;
> +
> +    if (remoteDeserializeTypedParameters(msg->params.params_val,
> +                                         msg->params.params_len,
> +                                         REMOTE_CPUMAPS_MAX,
> +                                         &params, &nparams) < 0)
> +        goto cleanup;
> +
> +    event = virDomainEventCputuneNewFromDom(dom, params, nparams);
> +

Memory leak - if event is NULL because of OOM, you leaked params.
Again, goes back to my point that if you WANT transfer semantics, then
virDomainEventCputuneNewFromDom must free params even on failure, to
make life easier on callers.

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

signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list