Re: [libvirt] [PATCH v2 3/5] event: introduce new event for cputune
On 09/12/2014 09:46 PM, John Ferlan wrote:
On 09/10/2014 08: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 <phrdina(a)redhat.com>
> ---
> daemon/remote.c | 45 ++++++++++++++++++++++++
> include/libvirt/libvirt.h.in | 19 ++++++++++
> src/conf/domain_event.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_event.h | 9 +++++
> src/libvirt_private.syms | 2 ++
> src/remote/remote_driver.c | 41 +++++++++++++++++++++
> src/remote/remote_protocol.x | 14 +++++++-
> src/remote_protocol-structs | 9 +++++
> tools/virsh-domain.c | 33 +++++++++++++++++
> 9 files changed, 255 insertions(+), 1 deletion(-)
>
Looks OK to me - there one detail below...
[..]
>
> 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,
> + ¶ms, &nparams) < 0)
> + goto cleanup;
> +
> + event = virDomainEventCputuneNewFromDom(dom, params, nparams);
> +
> + remoteEventQueue(priv, event, msg->callbackID);
> +
> + cleanup:
> + virDomainFree(dom);
Every other calling sequence as DomainFree followed by EventQueue - I
have to believe there's a reason - although nothing pops right out.
Like I said above in general ACK as I don't see anything obvious. It
certainly seems easier for me to add iothreadpin events into my code.
John
Yes that's true that every other event has virDomanFree followed by
remoteEventQueue and the only reason I cant think of could be that
virDomainFree is resetting last error, but I'm OK with changing the
calling sequence.
Pavel