On 09/18/2014 04:39 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 any tunable
> values or add new tunable values. With typedParameters we don't have to
> worry about creating some other events, we will just use this universal
> event to inform user about updates.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
> daemon/remote.c | 45 +++++++++++++++++++++
> include/libvirt/libvirt.h.in | 22 +++++++++++
> src/conf/domain_event.c | 93 ++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_event.h | 9 +++++
> src/libvirt_private.syms | 2 +
> src/remote/remote_driver.c | 42 ++++++++++++++++++++
> src/remote/remote_protocol.x | 14 ++++++-
> src/remote_protocol-structs | 9 +++++
> tools/virsh-domain.c | 33 ++++++++++++++++
> 9 files changed, 268 insertions(+), 1 deletion(-)
>
Seems you covered previous code review comments just fine, although I
have a couple of questions
#1. Is virsh-domain.c relative here or perhaps later? Or should the
verb/action be changed from "cpu-tune" to something more generic?
"tunable"?
#2. Should the REMOTE_CPUMAPS_MAX be replaced by something more name
(and size) relative to these events? And secondarily, since event max
params is relevant to the size of the params buffer allocated for each
event series shouldn't that be passed along and eventually used as the
limit for the remoteDeserializeTypedParameters? Although I'm not quite
clear/sure how that works with the protocol definition file.
I think this is ACK-able - just want to make sure of the above - #1
should be 'simple' right? #2 is just one of those size things - the
current usage goes on a byte max, but is that relative to how this has
changed/evolved.
John
> diff --git a/daemon/remote.c b/daemon/remote.c
> index daa4b60..ddd510c 100644
> --- a/daemon/remote.c
> +++ 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);
> +
> +static int
> remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
> int nerrors,
> remote_domain_disk_error **ret_errors_val,
> @@ -969,6 +976,43 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn,
> }
>
>
> +static int
> +remoteRelayDomainEventTunable(virConnectPtr conn,
> + virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams,
> + void *opaque)
> +{
> + daemonClientEventCallbackPtr callback = opaque;
> + remote_domain_event_callback_tunable_msg data;
> +
> + if (callback->callbackID < 0 ||
> + !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
> + return -1;
> +
> + VIR_DEBUG("Relaying domain tunable event %s %d, callback %d",
> + dom->name, dom->id, callback->callbackID);
> +
> + /* build return data */
> + memset(&data, 0, sizeof(data));
> + data.callbackID = callback->callbackID;
> + make_nonnull_domain(&data.dom, dom);
> +
> + if (remoteSerializeTypedParameters(params, nparams,
> + &data.params.params_val,
> + &data.params.params_len,
> + VIR_TYPED_PARAM_STRING_OKAY) < 0)
> + return -1;
> +
> + remoteDispatchObjectEventSend(callback->client, remoteProgram,
> + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE,
> + (xdrproc_t)xdr_remote_domain_event_callback_tunable_msg,
> + &data);
> +
> + return 0;
> +}
> +
> +
> static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot),
> @@ -987,6 +1031,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2),
> + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventTunable),
> };
>
> verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 724314e..4149596 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -5177,6 +5177,27 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
> const char *devAlias,
> void *opaque);
>
> +/**
> + * virConnectDomainEventTunableCallback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @params: changed tunable values stored as array of virTypedParameter
> + * @nparams: size of the array
> + * @opaque: application specified data
> + *
> + * This callback occurs when tunable values are updated. The params must not
> + * be freed in the callback handler as it's done internally after the callback
> + * handler is executed.
> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_TUNABLE with virConnectDomainEventRegisterAny()
> + */
> +typedef void (*virConnectDomainEventTunableCallback)(virConnectPtr conn,
> + virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams,
> + void *opaque);
> +
>
> /**
> * VIR_DOMAIN_EVENT_CALLBACK:
> @@ -5212,6 +5233,7 @@ typedef enum {
> VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK = 14, /* virConnectDomainEventPMSuspendDiskCallback */
> VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED = 15, /* virConnectDomainEventDeviceRemovedCallback */
> VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 = 16, /* virConnectDomainEventBlockJobCallback */
> + VIR_DOMAIN_EVENT_ID_TUNABLE = 17, /* virConnectDomainEventTunableCallback */
>
> #ifdef VIR_ENUM_SENTINELS
> VIR_DOMAIN_EVENT_ID_LAST
> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
> index 73ae289..bf187cd 100644
> --- a/src/conf/domain_event.c
> +++ b/src/conf/domain_event.c
> @@ -34,6 +34,7 @@
> #include "viralloc.h"
> #include "virerror.h"
> #include "virstring.h"
> +#include "virtypedparam.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -52,6 +53,7 @@ static virClassPtr virDomainEventBalloonChangeClass;
> static virClassPtr virDomainEventDeviceRemovedClass;
> static virClassPtr virDomainEventPMClass;
> static virClassPtr virDomainQemuMonitorEventClass;
> +static virClassPtr virDomainEventTunableClass;
>
>
> static void virDomainEventDispose(void *obj);
> @@ -67,6 +69,7 @@ static void virDomainEventBalloonChangeDispose(void *obj);
> static void virDomainEventDeviceRemovedDispose(void *obj);
> static void virDomainEventPMDispose(void *obj);
> static void virDomainQemuMonitorEventDispose(void *obj);
> +static void virDomainEventTunableDispose(void *obj);
>
> static void
> virDomainEventDispatchDefaultFunc(virConnectPtr conn,
> @@ -203,6 +206,15 @@ struct _virDomainQemuMonitorEvent {
> typedef struct _virDomainQemuMonitorEvent virDomainQemuMonitorEvent;
> typedef virDomainQemuMonitorEvent *virDomainQemuMonitorEventPtr;
>
> +struct _virDomainEventTunable {
> + virDomainEvent parent;
> +
> + virTypedParameterPtr params;
> + int nparams;
> +};
> +typedef struct _virDomainEventTunable virDomainEventTunable;
> +typedef virDomainEventTunable *virDomainEventTunablePtr;
> +
>
> static int
> virDomainEventsOnceInit(void)
> @@ -285,6 +297,12 @@ virDomainEventsOnceInit(void)
> sizeof(virDomainQemuMonitorEvent),
> virDomainQemuMonitorEventDispose)))
> return -1;
> + if (!(virDomainEventTunableClass =
> + virClassNew(virDomainEventClass,
> + "virDomainEventTunable",
> + sizeof(virDomainEventTunable),
> + virDomainEventTunableDispose)))
> + return -1;
> return 0;
> }
>
> @@ -420,6 +438,15 @@ virDomainQemuMonitorEventDispose(void *obj)
> VIR_FREE(event->details);
> }
>
> +static void
> +virDomainEventTunableDispose(void *obj)
> +{
> + virDomainEventTunablePtr event = obj;
> + VIR_DEBUG("obj=%p", event);
> +
> + virTypedParamsFree(event->params, event->nparams);
> +}
> +
>
> static void *
> virDomainEventNew(virClassPtr klass,
> @@ -1175,6 +1202,61 @@ virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom,
> devAlias);
> }
>
> +/* This function consumes the params so caller don't have to care about
> + * freeing it even if error occurs. The reason is to not have to do deep
> + * copy of params.
> + */
> +static virObjectEventPtr
> +virDomainEventTunableNew(int id,
> + const char *name,
> + unsigned char *uuid,
> + virTypedParameterPtr params,
> + int nparams)
> +{
> + virDomainEventTunablePtr ev;
> +
> + if (virDomainEventsInitialize() < 0)
> + goto error;
> +
> + if (!(ev = virDomainEventNew(virDomainEventTunableClass,
> + VIR_DOMAIN_EVENT_ID_TUNABLE,
> + id, name, uuid)))
> + goto error;
> +
> + ev->params = params;
> + ev->nparams = nparams;
> +
> + return (virObjectEventPtr)ev;
> +
> + error:
> + virTypedParamsFree(params, nparams);
> + return NULL;
> +}
> +
> +virObjectEventPtr
> +virDomainEventTunableNewFromObj(virDomainObjPtr obj,
> + virTypedParameterPtr params,
> + int nparams)
> +{
> + return virDomainEventTunableNew(obj->def->id,
> + obj->def->name,
> + obj->def->uuid,
> + params,
> + nparams);
> +}
> +
> +virObjectEventPtr
> +virDomainEventTunableNewFromDom(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams)
> +{
> + return virDomainEventTunableNew(dom->id,
> + dom->name,
> + dom->uuid,
> + params,
> + nparams);
> +}
> +
>
> static void
> virDomainEventDispatchDefaultFunc(virConnectPtr conn,
> @@ -1366,6 +1448,17 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn,
> goto cleanup;
> }
>
> + case VIR_DOMAIN_EVENT_ID_TUNABLE:
> + {
> + virDomainEventTunablePtr tunableEvent;
> + tunableEvent = (virDomainEventTunablePtr)event;
> + ((virConnectDomainEventTunableCallback)cb)(conn, dom,
> + tunableEvent->params,
> + tunableEvent->nparams,
> + cbopaque);
> + goto cleanup;
> + }
> +
> case VIR_DOMAIN_EVENT_ID_LAST:
> break;
> }
> diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
> index a3330ca..dc0109c 100644
> --- a/src/conf/domain_event.h
> +++ b/src/conf/domain_event.h
> @@ -184,6 +184,15 @@ virDomainEventDeviceRemovedNewFromObj(virDomainObjPtr obj,
> virObjectEventPtr
> virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom,
> const char *devAlias);
> +virObjectEventPtr
> +virDomainEventTunableNewFromObj(virDomainObjPtr obj,
> + virTypedParameterPtr params,
> + int nparams);
> +virObjectEventPtr
> +virDomainEventTunableNewFromDom(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams);
> +
>
> int
> virDomainEventStateRegister(virConnectPtr conn,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 51a692b..a339ced 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -473,6 +473,8 @@ virDomainEventStateRegister;
> virDomainEventStateRegisterID;
> virDomainEventTrayChangeNewFromDom;
> virDomainEventTrayChangeNewFromObj;
> +virDomainEventTunableNewFromDom;
> +virDomainEventTunableNewFromObj;
> virDomainEventWatchdogNewFromDom;
> virDomainEventWatchdogNewFromObj;
> virDomainQemuMonitorEventNew;
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 75a3a7b..79e3fd4 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -331,6 +331,11 @@ remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog,
> void *evdata, void *opaque);
>
> static void
> +remoteDomainBuildEventCallbackTunable(virNetClientProgramPtr prog,
> + virNetClientPtr client,
> + void *evdata, void *opaque);
> +
> +static void
> remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED,
> virNetClientPtr client ATTRIBUTE_UNUSED,
> void *evdata, void *opaque);
> @@ -481,6 +486,10 @@ static virNetClientProgramEvent remoteEvents[] = {
> remoteDomainBuildEventBlockJob2,
> sizeof(remote_domain_event_block_job_2_msg),
> (xdrproc_t)xdr_remote_domain_event_block_job_2_msg },
> + { REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE,
> + remoteDomainBuildEventCallbackTunable,
> + sizeof(remote_domain_event_callback_tunable_msg),
> + (xdrproc_t)xdr_remote_domain_event_callback_tunable_msg },
> };
>
>
> @@ -5514,6 +5523,39 @@ remoteDomainBuildEventCallbackDeviceRemoved(virNetClientProgramPtr prog ATTRIBUT
>
>
> static void
> +remoteDomainBuildEventCallbackTunable(virNetClientProgramPtr prog ATTRIBUTE_UNUSED,
> + virNetClientPtr client ATTRIBUTE_UNUSED,
> + void *evdata, void *opaque)
> +{
> + virConnectPtr conn = opaque;
> + remote_domain_event_callback_tunable_msg *msg = evdata;
> + struct private_data *priv = conn->privateData;
> + virDomainPtr dom;
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> + virObjectEventPtr event = NULL;
> +
> + if (remoteDeserializeTypedParameters(msg->params.params_val,
> + msg->params.params_len,
> + REMOTE_CPUMAPS_MAX,
> + ¶ms, &nparams) < 0)
> + return;
> +
> + dom = get_nonnull_domain(conn, msg->dom);
> + if (!dom) {
> + virTypedParamsFree(params, nparams);
> + return;
> + }
> +
> + event = virDomainEventTunableNewFromDom(dom, params, nparams);
> +
> + virDomainFree(dom);
> +
> + remoteEventQueue(priv, event, msg->callbackID);
> +}
> +
> +
> +static void
> remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED,
> virNetClientPtr client ATTRIBUTE_UNUSED,
> void *evdata, void *opaque)
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index a4ca0c3..324d9e4 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -2990,6 +2990,12 @@ struct remote_domain_event_block_job_2_msg {
> int status;
> };
>
> +struct remote_domain_event_callback_tunable_msg {
> + int callbackID;
> + remote_nonnull_domain dom;
> + remote_typed_param params<REMOTE_CPUMAPS_MAX>;
> +};
> +
> struct remote_connect_get_cpu_model_names_args {
> remote_nonnull_string arch;
> int need_results;
> @@ -5472,5 +5478,11 @@ enum remote_procedure {
> * @generate: both
> * @acl: domain:block_write
> */
> - REMOTE_PROC_DOMAIN_BLOCK_COPY = 345
> + REMOTE_PROC_DOMAIN_BLOCK_COPY = 345,
> +
> + /**
> + * @generate: both
> + * @acl: none
> + */
> + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE = 346
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index e960415..6128a85 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -2445,6 +2445,14 @@ struct remote_domain_event_block_job_2_msg {
> int type;
> int status;
> };
> +struct remote_domain_event_callback_tunable_msg {
> + int callbackID;
> + remote_nonnull_domain dom;
> + struct {
> + u_int params_len;
> + remote_typed_param * params_val;
> + } params;
> +};
> struct remote_connect_get_cpu_model_names_args {
> remote_nonnull_string arch;
> int need_results;
> @@ -2901,4 +2909,5 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_OPEN_GRAPHICS_FD = 343,
> REMOTE_PROC_CONNECT_GET_ALL_DOMAIN_STATS = 344,
> REMOTE_PROC_DOMAIN_BLOCK_COPY = 345,
> + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE = 346,
> };
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 435d045..cbf323a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11420,6 +11420,37 @@ vshEventDeviceRemovedPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
> vshEventDone(data->ctl);
> }
>
> +static void
> +vshEventTunablePrint(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams,
> + void *opaque)
> +{
> + vshDomEventData *data = opaque;
> + size_t i;
> + char *value = NULL;
> +
> + if (!data->loop && *data->count)
> + return;
> +
> + vshPrint(data->ctl,
> + _("event 'tunable' for domain %s:\n"),
> + virDomainGetName(dom));
> +
> + for (i = 0; i < nparams; i++) {
> + value = virTypedParameterToString(¶ms[i]);
> + if (value) {
> + vshPrint(data->ctl, _("\t%s: %s\n"), params[i].field, value);
> + VIR_FREE(value);
> + }
> + }
> +
> + (*data->count)++;
> + if (!data->loop)
> + vshEventDone(data->ctl);
> +}
> +
> static vshEventCallback vshEventCallbacks[] = {
> { "lifecycle",
> VIR_DOMAIN_EVENT_CALLBACK(vshEventLifecyclePrint), },
> @@ -11453,6 +11484,8 @@ static vshEventCallback vshEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(vshEventDeviceRemovedPrint), },
> { "block-job-2",
> VIR_DOMAIN_EVENT_CALLBACK(vshEventBlockJobPrint), },
> + { "cpu-tune",
> + VIR_DOMAIN_EVENT_CALLBACK(vshEventTunablePrint), },
> };
> verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks));
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list