[libvirt] [PATCH v2 0/5] Introduce new cputune event
This patch series introduce new cputune event to inform management applications about every change of cputune values for running domains. The first patch introduces a new string helper virSnprintf and the second patch cleans up the virDomanDef structure. There is missing documentation for all events so the documentation for this event will be part of the patches to document all events. Changes from v1: - new virSnprintf helper - complete cleanup of virDomainDef structure - cputune event now uses virTypedParameter Pavel Hrdina (5): introduce virSnprintf helper domain_conf: separate structures from virDomainDef event: introduce new event for cputune add an example how to use cputune event cputune_event: queue the event for cputune updates daemon/remote.c | 45 ++++++++++++++++++ examples/object-events/event-test.c | 52 +++++++++++++++++++- include/libvirt/libvirt.h.in | 19 ++++++++ src/conf/domain_conf.h | 94 ++++++++++++++++++++++--------------- src/conf/domain_event.c | 84 +++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 9 ++++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_cgroup.c | 18 ++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++ src/remote/remote_driver.c | 41 ++++++++++++++++ src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 9 ++++ src/util/virstring.c | 25 ++++++++++ src/util/virstring.h | 20 ++++++++ tools/virsh-domain.c | 33 +++++++++++++ 15 files changed, 499 insertions(+), 40 deletions(-) -- 1.8.5.5
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 25 +++++++++++++++++++++++++ src/util/virstring.h | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf4548..80d8e7d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1956,6 +1956,7 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virSnprintfInternal; virStrcpy; virStrdup; virStringArrayHasString; diff --git a/src/util/virstring.c b/src/util/virstring.c index b14f785..2a657b3 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -483,6 +483,31 @@ virAsprintfInternal(bool report, return ret; } +int +virSnprintfInternal(int domcode, + const char *filename, + const char *funcname, + size_t linenr, + char *strp, + size_t strLen, + const char *fmt, ...) +{ + va_list ap; + int ret; + + va_start(ap, fmt); + ret = vsnprintf(strp, strLen, fmt, ap); + + if (ret >= strLen) + virReportErrorHelper(domcode, VIR_ERR_INTERNAL_ERROR, + filename, funcname, linenr, + _("Size of src string '%d' is greater than available size of dst string '%zu'"), + ret, strLen); + va_end(ap); + return ret; + +} + /** * virStrncpy * diff --git a/src/util/virstring.h b/src/util/virstring.h index 267fbd0..c526c12 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -129,6 +129,11 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, const char *fmt, va_list list) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7) ATTRIBUTE_FMT_PRINTF(7, 0) ATTRIBUTE_RETURN_CHECK; +int virSnprintfInternal(int domcode, const char *filename, const char *funcname, + size_t linenr, char *strp, size_t strLen, + const char *fmt, ...) + ATTRIBUTE_NONNULL(5) ATTRIBUTE_FMT_PRINTF(7, 8) + ATTRIBUTE_RETURN_CHECK; /** * VIR_STRDUP: @@ -252,6 +257,21 @@ size_t virStringListLength(char **strings); virAsprintfInternal(false, 0, NULL, NULL, 0, \ strp, __VA_ARGS__) +/** + * virSnprintf: + * @strp: variable to hold result (char*) + * @fmt: printf format + * + * Like glibc's_snprintf but report error if the src string is longer + * then dst string. + * + * Returns -1 on failure, number of bytes printed on success. + */ + +# define virSnprintf(strp, ...) \ + virSnprintfInternal(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__, \ + strp, ARRAY_CARDINALITY(strp), __VA_ARGS__) + int virStringSortCompare(const void *a, const void *b); int virStringSortRevCompare(const void *a, const void *b); -- 1.8.5.5
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 25 +++++++++++++++++++++++++ src/util/virstring.h | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+)
Not sure why this is necessary - I do see calls to 'snprintf' from within existing libvirt code. The usage you have in patch 5 could easily use snprintf. There is no need for variable argument list. I'm not against it, but if it's created shouldn't other snprintf's use it? Hopefully someone else has an opinion on this... John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf4548..80d8e7d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1956,6 +1956,7 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virSnprintfInternal; virStrcpy; virStrdup; virStringArrayHasString; diff --git a/src/util/virstring.c b/src/util/virstring.c index b14f785..2a657b3 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -483,6 +483,31 @@ virAsprintfInternal(bool report, return ret; }
+int +virSnprintfInternal(int domcode, + const char *filename, + const char *funcname, + size_t linenr, + char *strp, + size_t strLen, + const char *fmt, ...) +{ + va_list ap; + int ret; + + va_start(ap, fmt); + ret = vsnprintf(strp, strLen, fmt, ap); + + if (ret >= strLen) + virReportErrorHelper(domcode, VIR_ERR_INTERNAL_ERROR, + filename, funcname, linenr, + _("Size of src string '%d' is greater than available size of dst string '%zu'"), + ret, strLen); + va_end(ap); + return ret; + +} + /** * virStrncpy * diff --git a/src/util/virstring.h b/src/util/virstring.h index 267fbd0..c526c12 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -129,6 +129,11 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, const char *fmt, va_list list) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7) ATTRIBUTE_FMT_PRINTF(7, 0) ATTRIBUTE_RETURN_CHECK; +int virSnprintfInternal(int domcode, const char *filename, const char *funcname, + size_t linenr, char *strp, size_t strLen, + const char *fmt, ...) + ATTRIBUTE_NONNULL(5) ATTRIBUTE_FMT_PRINTF(7, 8) + ATTRIBUTE_RETURN_CHECK;
/** * VIR_STRDUP: @@ -252,6 +257,21 @@ size_t virStringListLength(char **strings); virAsprintfInternal(false, 0, NULL, NULL, 0, \ strp, __VA_ARGS__)
+/** + * virSnprintf: + * @strp: variable to hold result (char*) + * @fmt: printf format + * + * Like glibc's_snprintf but report error if the src string is longer + * then dst string. + * + * Returns -1 on failure, number of bytes printed on success. + */ + +# define virSnprintf(strp, ...) \ + virSnprintfInternal(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__, \ + strp, ARRAY_CARDINALITY(strp), __VA_ARGS__) + int virStringSortCompare(const void *a, const void *b); int virStringSortRevCompare(const void *a, const void *b);
On 09/12/2014 12:47 PM, John Ferlan wrote:
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 25 +++++++++++++++++++++++++ src/util/virstring.h | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+)
Not sure why this is necessary - I do see calls to 'snprintf' from within existing libvirt code. The usage you have in patch 5 could easily use snprintf. There is no need for variable argument list.
I'm not against it, but if it's created shouldn't other snprintf's use it?
I agree that if we introduce this, we should add a syntax check that forbids raw use of snprintf, and fix all existing callers to start using it. It does have the nice aspect of doing automatic error reporting if the buffer is too small, so I'm not opposed to adding it, but it would need another version. I'm also okay if you just directly use snprintf (seeing as how we already have existing code that does that) without trying to add this wrapper.
+/** + * virSnprintf: + * @strp: variable to hold result (char*) + * @fmt: printf format + * + * Like glibc's_snprintf but report error if the src string is longer + * then dst string.
Wouldn't that really be "report error if the resulting output is too long for the dst string"?
+ * + * Returns -1 on failure, number of bytes printed on success. + */ + +# define virSnprintf(strp, ...) \ + virSnprintfInternal(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__, \ + strp, ARRAY_CARDINALITY(strp), __VA_ARGS__)
Hmm, this doesn't match your documentation. ARRAY_CARDINALITY only works on char[], not on char* (that is, the compiler has to know how long the array is, not just that it is a pointer). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/12/2014 08:47 PM, John Ferlan wrote:
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 25 +++++++++++++++++++++++++ src/util/virstring.h | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+)
Not sure why this is necessary - I do see calls to 'snprintf' from within existing libvirt code. The usage you have in patch 5 could easily use snprintf. There is no need for variable argument list.
I'm not against it, but if it's created shouldn't other snprintf's use it?
Hopefully someone else has an opinion on this...
John
OK, I'll drop it for this patch series. [..]
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.h | 94 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index efae2f5..c82b224 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1894,6 +1894,59 @@ struct _virDomaiHugePage { unsigned long long size; /* hugepage size in KiB */ }; +typedef struct _virDomainCputune virDomainCputune; +typedef virDomainCputune *virDomainCputunePtr; + +struct _virDomainCputune { + unsigned long shares; + bool sharesSpecified; + unsigned long long period; + long long quota; + unsigned long long emulator_period; + long long emulator_quota; + size_t nvcpupin; + virDomainVcpuPinDefPtr *vcpupin; + virDomainVcpuPinDefPtr emulatorpin; +}; + +typedef struct _virDomainBlkiotune virDomainBlkiotune; +typedef virDomainBlkiotune *virDomainBlkiotunePtr; + +struct _virDomainBlkiotune { + unsigned int weight; + + size_t ndevices; + virBlkioDevicePtr devices; +}; + +typedef struct _virDomainMemtune virDomainMemtune; +typedef virDomainMemtune *virDomainMemtunePtr; + +struct _virDomainMemtune { + unsigned long long max_balloon; /* in kibibytes */ + unsigned long long cur_balloon; /* in kibibytes */ + + virDomainHugePagePtr hugepages; + size_t nhugepages; + + bool nosharepages; + bool locked; + int dump_core; /* enum virTristateSwitch */ + unsigned long long hard_limit; /* in kibibytes */ + unsigned long long soft_limit; /* in kibibytes */ + unsigned long long min_guarantee; /* in kibibytes */ + unsigned long long swap_hard_limit; /* in kibibytes */ +}; + +typedef struct _virDomainPM virDomainPM; +typedef virDomainPM *virDomainPMPtr; + +struct _virDomainPM { + /* These options are of type enum virTristateBool */ + int s3; + int s4; +}; + /* * Guest VM main configuration * @@ -1910,28 +1963,9 @@ struct _virDomainDef { char *title; char *description; - struct { - unsigned int weight; - - size_t ndevices; - virBlkioDevicePtr devices; - } blkio; + virDomainBlkiotune blkio; + virDomainMemtune mem; - struct { - unsigned long long max_balloon; /* in kibibytes */ - unsigned long long cur_balloon; /* in kibibytes */ - - virDomainHugePagePtr hugepages; - size_t nhugepages; - - bool nosharepages; - bool locked; - int dump_core; /* enum virTristateSwitch */ - unsigned long long hard_limit; /* in kibibytes */ - unsigned long long soft_limit; /* in kibibytes */ - unsigned long long min_guarantee; /* in kibibytes */ - unsigned long long swap_hard_limit; /* in kibibytes */ - } mem; unsigned short vcpus; unsigned short maxvcpus; int placement_mode; @@ -1939,17 +1973,7 @@ struct _virDomainDef { unsigned int iothreads; - struct { - unsigned long shares; - bool sharesSpecified; - unsigned long long period; - long long quota; - unsigned long long emulator_period; - long long emulator_quota; - size_t nvcpupin; - virDomainVcpuPinDefPtr *vcpupin; - virDomainVcpuPinDefPtr emulatorpin; - } cputune; + virDomainCputune cputune; virDomainNumatunePtr numatune; virDomainResourceDefPtr resource; @@ -1962,11 +1986,7 @@ struct _virDomainDef { int onLockFailure; /* enum virDomainLockFailureAction */ - struct { - /* These options are of type enum virTristateBool */ - int s3; - int s4; - } pm; + virDomainPM pm; virDomainOSDef os; char *emulator; -- 1.8.5.5
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.h | 94 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 37 deletions(-)
Little light on the description. There are those that would prefer 3 patches - one for each structure... The Blkiotune and PM are unrelated...
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index efae2f5..c82b224 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1894,6 +1894,59 @@ struct _virDomaiHugePage {
Hmmmm... Seeing as you're fixing things - do you see it? would't it be ice to fix this oe too? :-)
unsigned long long size; /* hugepage size in KiB */ };
+typedef struct _virDomainCputune virDomainCputune; +typedef virDomainCputune *virDomainCputunePtr; + +struct _virDomainCputune { + unsigned long shares; + bool sharesSpecified; + unsigned long long period; + long long quota; + unsigned long long emulator_period; + long long emulator_quota; + size_t nvcpupin; + virDomainVcpuPinDefPtr *vcpupin; + virDomainVcpuPinDefPtr emulatorpin; +}; + +typedef struct _virDomainBlkiotune virDomainBlkiotune; +typedef virDomainBlkiotune *virDomainBlkiotunePtr; + +struct _virDomainBlkiotune { + unsigned int weight; + + size_t ndevices; + virBlkioDevicePtr devices; +}; + +typedef struct _virDomainMemtune virDomainMemtune; +typedef virDomainMemtune *virDomainMemtunePtr; + +struct _virDomainMemtune { + unsigned long long max_balloon; /* in kibibytes */ + unsigned long long cur_balloon; /* in kibibytes */ + + virDomainHugePagePtr hugepages; + size_t nhugepages; + + bool nosharepages; + bool locked; + int dump_core; /* enum virTristateSwitch */ + unsigned long long hard_limit; /* in kibibytes */ + unsigned long long soft_limit; /* in kibibytes */ + unsigned long long min_guarantee; /* in kibibytes */ + unsigned long long swap_hard_limit; /* in kibibytes */ +}; + +typedef struct _virDomainPM virDomainPM; +typedef virDomainPM *virDomainPMPtr; + +struct _virDomainPM {
s/PM/PowerManagement
+ /* These options are of type enum virTristateBool */ + int s3; + int s4; +}; + /* * Guest VM main configuration * @@ -1910,28 +1963,9 @@ struct _virDomainDef { char *title; char *description;
- struct { - unsigned int weight; - - size_t ndevices; - virBlkioDevicePtr devices; - } blkio; + virDomainBlkiotune blkio; + virDomainMemtune mem;
- struct { - unsigned long long max_balloon; /* in kibibytes */ - unsigned long long cur_balloon; /* in kibibytes */ - - virDomainHugePagePtr hugepages; - size_t nhugepages; - - bool nosharepages; - bool locked; - int dump_core; /* enum virTristateSwitch */ - unsigned long long hard_limit; /* in kibibytes */ - unsigned long long soft_limit; /* in kibibytes */ - unsigned long long min_guarantee; /* in kibibytes */ - unsigned long long swap_hard_limit; /* in kibibytes */ - } mem; unsigned short vcpus; unsigned short maxvcpus; int placement_mode; @@ -1939,17 +1973,7 @@ struct _virDomainDef {
unsigned int iothreads;
- struct { - unsigned long shares; - bool sharesSpecified; - unsigned long long period; - long long quota; - unsigned long long emulator_period; - long long emulator_quota; - size_t nvcpupin; - virDomainVcpuPinDefPtr *vcpupin; - virDomainVcpuPinDefPtr emulatorpin; - } cputune; + virDomainCputune cputune;
virDomainNumatunePtr numatune; virDomainResourceDefPtr resource; @@ -1962,11 +1986,7 @@ struct _virDomainDef {
int onLockFailure; /* enum virDomainLockFailureAction */
- struct { - /* These options are of type enum virTristateBool */ - int s3; - int s4; - } pm; + virDomainPM pm;
virDomainOSDef os; char *emulator;
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@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(-) diff --git a/daemon/remote.c b/daemon/remote.c index 0ea2815..798738e 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 +remoteRelayDomainEventCputune(virConnectPtr conn, + virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + void *opaque) +{ + daemonClientEventCallbackPtr callback = opaque; + remote_domain_event_callback_cputune_msg data; + + if (callback->callbackID < 0 || + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) + return -1; + + VIR_DEBUG("Relaying domain cputune 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_CPUTUNE, + (xdrproc_t)xdr_remote_domain_event_callback_cputune_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(remoteRelayDomainEventCputune), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..cf50662 100644 --- a/include/libvirt/libvirt.h.in +++ 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. + * + * 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); /** * VIR_DOMAIN_EVENT_CALLBACK: @@ -5188,6 +5206,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_CPUTUNE = 17, /* virConnectDomainEventCputuneCallback */ #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..990ebec 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 virDomainEventCputuneClass; 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 virDomainEventCputuneDispose(void *obj); static void virDomainEventDispatchDefaultFunc(virConnectPtr conn, @@ -203,6 +206,15 @@ struct _virDomainQemuMonitorEvent { typedef struct _virDomainQemuMonitorEvent virDomainQemuMonitorEvent; typedef virDomainQemuMonitorEvent *virDomainQemuMonitorEventPtr; +struct _virDomainEventCputune { + virDomainEvent parent; + + virTypedParameterPtr params; + int nparams; +}; +typedef struct _virDomainEventCputune virDomainEventCputune; +typedef virDomainEventCputune *virDomainEventCputunePtr; + static int virDomainEventsOnceInit(void) @@ -285,6 +297,12 @@ virDomainEventsOnceInit(void) sizeof(virDomainQemuMonitorEvent), virDomainQemuMonitorEventDispose))) return -1; + if (!(virDomainEventCputuneClass = + virClassNew(virDomainEventClass, + "virDomainEventCputune", + sizeof(virDomainEventCputune), + virDomainEventCputuneDispose))) + return -1; return 0; } @@ -420,6 +438,15 @@ virDomainQemuMonitorEventDispose(void *obj) VIR_FREE(event->details); } +static void +virDomainEventCputuneDispose(void *obj) +{ + virDomainEventCputunePtr event = obj; + VIR_DEBUG("obj=%p", event); + + virTypedParamsFree(event->params, event->nparams); +} + static void * virDomainEventNew(virClassPtr klass, @@ -1175,6 +1202,52 @@ virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom, devAlias); } +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; + + return (virObjectEventPtr)ev; +} + +virObjectEventPtr +virDomainEventCputuneNewFromObj(virDomainObjPtr obj, + virTypedParameterPtr params, + int nparams) +{ + return virDomainEventCputuneNew(obj->def->id, + obj->def->name, + obj->def->uuid, + params, + nparams); +} + +virObjectEventPtr +virDomainEventCputuneNewFromDom(virDomainPtr dom, + virTypedParameterPtr params, + int nparams) +{ + return virDomainEventCputuneNew(dom->id, + dom->name, + dom->uuid, + params, + nparams); +} static void virDomainEventDispatchDefaultFunc(virConnectPtr conn, @@ -1366,6 +1439,17 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, goto cleanup; } + case VIR_DOMAIN_EVENT_ID_CPUTUNE: + { + virDomainEventCputunePtr cputuneEvent; + cputuneEvent = (virDomainEventCputunePtr)event; + ((virConnectDomainEventCputuneCallback)cb)(conn, dom, + cputuneEvent->params, + cputuneEvent->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..bf8c30e 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 +virDomainEventCputuneNewFromObj(virDomainObjPtr obj, + virTypedParameterPtr params, + int nparams); +virObjectEventPtr +virDomainEventCputuneNewFromDom(virDomainPtr dom, + virTypedParameterPtr params, + int nparams); + int virDomainEventStateRegister(virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 80d8e7d..0344fc2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -443,6 +443,8 @@ virDomainEventBlockJobNewFromDom; virDomainEventBlockJobNewFromObj; virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; +virDomainEventCputuneNewFromDom; +virDomainEventCputuneNewFromObj; virDomainEventDeviceRemovedNewFromDom; virDomainEventDeviceRemovedNewFromObj; virDomainEventDiskChangeNewFromDom; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8221683..9538c00 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -326,6 +326,11 @@ remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog, void *evdata, void *opaque); static void +remoteDomainBuildEventCallbackCputune(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + +static void remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, void *evdata, void *opaque); @@ -476,6 +481,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_CPUTUNE, + remoteDomainBuildEventCallbackCputune, + sizeof(remote_domain_event_callback_cputune_msg), + (xdrproc_t)xdr_remote_domain_event_callback_cputune_msg }, }; @@ -5500,6 +5509,38 @@ remoteDomainBuildEventCallbackDeviceRemoved(virNetClientProgramPtr prog ATTRIBUT 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); +} + + +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..7e13822 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_cputune_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_CPUTUNE = 346 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index e960415..d878874 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_cputune_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_CPUTUNE = 346, }; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 86deae6..d14d1b7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11401,6 +11401,37 @@ vshEventDeviceRemovedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, vshEventDone(data->ctl); } +static void +vshEventCputunePrint(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 'cpu-tune' 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), }, @@ -11434,6 +11465,8 @@ static vshEventCallback vshEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(vshEventDeviceRemovedPrint), }, { "block-job-2", VIR_DOMAIN_EVENT_CALLBACK(vshEventBlockJobPrint), }, + { "cpu-tune", + VIR_DOMAIN_EVENT_CALLBACK(vshEventCputunePrint), }, }; verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks)); -- 1.8.5.5
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@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...
diff --git a/daemon/remote.c b/daemon/remote.c index 0ea2815..798738e 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 +remoteRelayDomainEventCputune(virConnectPtr conn, + virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + void *opaque) +{ + daemonClientEventCallbackPtr callback = opaque; + remote_domain_event_callback_cputune_msg data; + + if (callback->callbackID < 0 || + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) + return -1; + + VIR_DEBUG("Relaying domain cputune 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_CPUTUNE, + (xdrproc_t)xdr_remote_domain_event_callback_cputune_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(remoteRelayDomainEventCputune), };
verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..cf50662 100644 --- a/include/libvirt/libvirt.h.in +++ 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. + * + * 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);
/** * VIR_DOMAIN_EVENT_CALLBACK: @@ -5188,6 +5206,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_CPUTUNE = 17, /* virConnectDomainEventCputuneCallback */
#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..990ebec 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 virDomainEventCputuneClass;
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 virDomainEventCputuneDispose(void *obj);
static void virDomainEventDispatchDefaultFunc(virConnectPtr conn, @@ -203,6 +206,15 @@ struct _virDomainQemuMonitorEvent { typedef struct _virDomainQemuMonitorEvent virDomainQemuMonitorEvent; typedef virDomainQemuMonitorEvent *virDomainQemuMonitorEventPtr;
+struct _virDomainEventCputune { + virDomainEvent parent; + + virTypedParameterPtr params; + int nparams; +}; +typedef struct _virDomainEventCputune virDomainEventCputune; +typedef virDomainEventCputune *virDomainEventCputunePtr; +
static int virDomainEventsOnceInit(void) @@ -285,6 +297,12 @@ virDomainEventsOnceInit(void) sizeof(virDomainQemuMonitorEvent), virDomainQemuMonitorEventDispose))) return -1; + if (!(virDomainEventCputuneClass = + virClassNew(virDomainEventClass, + "virDomainEventCputune", + sizeof(virDomainEventCputune), + virDomainEventCputuneDispose))) + return -1; return 0; }
@@ -420,6 +438,15 @@ virDomainQemuMonitorEventDispose(void *obj) VIR_FREE(event->details); }
+static void +virDomainEventCputuneDispose(void *obj) +{ + virDomainEventCputunePtr event = obj; + VIR_DEBUG("obj=%p", event); + + virTypedParamsFree(event->params, event->nparams); +} +
static void * virDomainEventNew(virClassPtr klass, @@ -1175,6 +1202,52 @@ virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom, devAlias); }
+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; + + return (virObjectEventPtr)ev; +} + +virObjectEventPtr +virDomainEventCputuneNewFromObj(virDomainObjPtr obj, + virTypedParameterPtr params, + int nparams) +{ + return virDomainEventCputuneNew(obj->def->id, + obj->def->name, + obj->def->uuid, + params, + nparams); +} + +virObjectEventPtr +virDomainEventCputuneNewFromDom(virDomainPtr dom, + virTypedParameterPtr params, + int nparams) +{ + return virDomainEventCputuneNew(dom->id, + dom->name, + dom->uuid, + params, + nparams); +}
static void virDomainEventDispatchDefaultFunc(virConnectPtr conn, @@ -1366,6 +1439,17 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, goto cleanup; }
+ case VIR_DOMAIN_EVENT_ID_CPUTUNE: + { + virDomainEventCputunePtr cputuneEvent; + cputuneEvent = (virDomainEventCputunePtr)event; + ((virConnectDomainEventCputuneCallback)cb)(conn, dom, + cputuneEvent->params, + cputuneEvent->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..bf8c30e 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 +virDomainEventCputuneNewFromObj(virDomainObjPtr obj, + virTypedParameterPtr params, + int nparams); +virObjectEventPtr +virDomainEventCputuneNewFromDom(virDomainPtr dom, + virTypedParameterPtr params, + int nparams); +
int virDomainEventStateRegister(virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 80d8e7d..0344fc2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -443,6 +443,8 @@ virDomainEventBlockJobNewFromDom; virDomainEventBlockJobNewFromObj; virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; +virDomainEventCputuneNewFromDom; +virDomainEventCputuneNewFromObj; virDomainEventDeviceRemovedNewFromDom; virDomainEventDeviceRemovedNewFromObj; virDomainEventDiskChangeNewFromDom; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8221683..9538c00 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -326,6 +326,11 @@ remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog, void *evdata, void *opaque);
static void +remoteDomainBuildEventCallbackCputune(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + +static void remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, void *evdata, void *opaque); @@ -476,6 +481,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_CPUTUNE, + remoteDomainBuildEventCallbackCputune, + sizeof(remote_domain_event_callback_cputune_msg), + (xdrproc_t)xdr_remote_domain_event_callback_cputune_msg }, };
@@ -5500,6 +5509,38 @@ remoteDomainBuildEventCallbackDeviceRemoved(virNetClientProgramPtr prog ATTRIBUT
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
+} + + +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..7e13822 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_cputune_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_CPUTUNE = 346 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index e960415..d878874 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_cputune_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_CPUTUNE = 346, }; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 86deae6..d14d1b7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11401,6 +11401,37 @@ vshEventDeviceRemovedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, vshEventDone(data->ctl); }
+static void +vshEventCputunePrint(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 'cpu-tune' 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), }, @@ -11434,6 +11465,8 @@ static vshEventCallback vshEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(vshEventDeviceRemovedPrint), }, { "block-job-2", VIR_DOMAIN_EVENT_CALLBACK(vshEventBlockJobPrint), }, + { "cpu-tune", + VIR_DOMAIN_EVENT_CALLBACK(vshEventCputunePrint), }, }; verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks));
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@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
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@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).
+++ 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@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
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 <phrdina@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, + ¶ms, &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
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- examples/object-events/event-test.c | 52 ++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index d6cfe46..f1b9cda 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -464,6 +464,48 @@ static int myNetworkEventCallback(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; } +static int +myDomainEventCputuneCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + void *opaque ATTRIBUTE_UNUSED) +{ + size_t i; + + printf("%s EVENT: Domain %s(%d) cputune updated:\n", + __func__, virDomainGetName(dom), virDomainGetID(dom)); + + for (i = 0; i < nparams; i++) { + switch (params[i].type) { + case VIR_TYPED_PARAM_INT: + printf("\t%s: %d\n", params[i].field, params[i].value.i); + break; + case VIR_TYPED_PARAM_UINT: + printf("\t%s: %u\n", params[i].field, params[i].value.ui); + break; + case VIR_TYPED_PARAM_LLONG: + printf("\t%s: %lld\n", params[i].field, params[i].value.l); + break; + case VIR_TYPED_PARAM_ULLONG: + printf("\t%s: %llu\n", params[i].field, params[i].value.ul); + break; + case VIR_TYPED_PARAM_DOUBLE: + printf("\t%s: %g\n", params[i].field, params[i].value.d); + break; + case VIR_TYPED_PARAM_BOOLEAN: + printf("\t%s: %d\n", params[i].field, params[i].value.b); + break; + case VIR_TYPED_PARAM_STRING: + printf("\t%s: %s\n", params[i].field, params[i].value.s); + break; + default: + printf("\t%s: unknown type\n", params[i].field); + } + } + + return 0; +} static void myFreeFunc(void *opaque) { @@ -506,6 +548,7 @@ int main(int argc, char **argv) int callback14ret = -1; int callback15ret = -1; int callback16ret = -1; + int callback17ret = -1; struct sigaction action_stop; memset(&action_stop, 0, sizeof(action_stop)); @@ -624,6 +667,11 @@ int main(int argc, char **argv) VIR_NETWORK_EVENT_ID_LIFECYCLE, VIR_NETWORK_EVENT_CALLBACK(myNetworkEventCallback), strdup("net callback"), myFreeFunc); + callback17ret = virConnectDomainEventRegisterAny(dconn, + NULL, + VIR_DOMAIN_EVENT_ID_CPUTUNE, + VIR_DOMAIN_EVENT_CALLBACK(myDomainEventCputuneCallback), + strdup("cputune"), myFreeFunc); if ((callback1ret != -1) && (callback2ret != -1) && @@ -639,7 +687,8 @@ int main(int argc, char **argv) (callback13ret != -1) && (callback14ret != -1) && (callback15ret != -1) && - (callback16ret != -1)) { + (callback16ret != -1) && + (callback17ret != -1)) { if (virConnectSetKeepAlive(dconn, 5, 3) < 0) { virErrorPtr err = virGetLastError(); fprintf(stderr, "Failed to start keepalive protocol: %s\n", @@ -671,6 +720,7 @@ int main(int argc, char **argv) virConnectDomainEventDeregisterAny(dconn, callback14ret); virConnectDomainEventDeregisterAny(dconn, callback15ret); virConnectNetworkEventDeregisterAny(dconn, callback16ret); + virConnectDomainEventDeregisterAny(dconn, callback17ret); if (callback8ret != -1) virConnectDomainEventDeregisterAny(dconn, callback8ret); } -- 1.8.5.5
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- examples/object-events/event-test.c | 52 ++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)
ACK John
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 18 ++++++++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 43d14d4..43b5340 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -34,6 +34,7 @@ #include "virscsi.h" #include "virstring.h" #include "virfile.h" +#include "virtypedparam.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -676,6 +677,10 @@ static int qemuSetupCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams; + int eventNparams = 0; + int eventMaxparams = 0; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { if (vm->def->cputune.sharesSpecified) { @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm) if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) return -1; - vm->def->cputune.shares = val; + if (vm->def->cputune.shares != val) { + vm->def->cputune.shares = val; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, "shares", + val) < 0) + return -1; + + event = virDomainEventCputuneNewFromObj(vm, eventParams, eventNparams); + } + + if (event) + qemuDomainEventQueue(vm->privateData, event); } return 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a8cda43..2389e32 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4462,6 +4462,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virBitmapPtr pcpumap = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4569,6 +4575,18 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + if (virSnprintf(paramField, "vcpu%d", vcpu) < 0) { + goto cleanup; + } + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) { + goto cleanup; + } + + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -4604,6 +4622,9 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virCgroupFree(&cgroup_vcpu); if (vm) virObjectUnlock(vm); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); virBitmapFree(pcpumap); virObjectUnref(caps); virObjectUnref(cfg); @@ -4728,6 +4749,12 @@ qemuDomainPinEmulator(virDomainPtr dom, virBitmapPtr pcpumap = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + virObjectEventPtr event = NULL; + char * str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4833,6 +4860,14 @@ qemuDomainPinEmulator(virDomainPtr dom, if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, "emulatorpin", str) < 0) { + goto cleanup; + } + + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -4862,6 +4897,9 @@ qemuDomainPinEmulator(virDomainPtr dom, cleanup: if (cgroup_emulator) virCgroupFree(&cgroup_emulator); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); virBitmapFree(pcpumap); virObjectUnref(caps); if (vm) @@ -9054,6 +9092,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxNparams = 0; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -9124,6 +9166,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "shares", + val) < 0) + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -9141,6 +9188,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; vm->def->cputune.period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "period", + value_ul) < 0) + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9155,6 +9207,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; vm->def->cputune.quota = value_l; + + if (virTypedParamsAddLLong(&eventParams, &eventNparams, + &eventMaxNparams, "quota", + value_l) < 0) + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9170,6 +9227,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; vm->def->cputune.emulator_period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "emulator_period", + value_ul) < 0) + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9185,6 +9247,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; vm->def->cputune.emulator_quota = value_l; + + if (virTypedParamsAddLLong(&eventParams, &eventNparams, + &eventMaxNparams, "emulator_quota", + value_l) < 0) + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vmdef = NULL; } + ret = 0; cleanup: virDomainDefFree(vmdef); if (vm) virObjectUnlock(vm); + if (eventNparams) { + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); + if (event) + qemuDomainEventQueue(driver, event); + } virObjectUnref(caps); virObjectUnref(cfg); return ret; -- 1.8.5.5
s//"something to describe what's being done!" Might be nice to see what the event would look like when it's triggered. May help in that documentation effort in the future. On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 18 ++++++++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-)
So this is only going to occur as asked in 4/4 in v1 when/if the cputune.shares value is adjusted during qemuProcessStart. Also events are only triggered when someone uses the api that virsh vcpupin, emulatorpin, and schedinfo(set) call. Which is "ok' by me for the onlist iothreadpin changes which don't yet have a similar iothreadpin api. Meaning my other series shouldn't be affected
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 43d14d4..43b5340 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -34,6 +34,7 @@ #include "virscsi.h" #include "virstring.h" #include "virfile.h" +#include "virtypedparam.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -676,6 +677,10 @@ static int qemuSetupCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams; + int eventNparams = 0; + int eventMaxparams = 0;
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { if (vm->def->cputune.sharesSpecified) { @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) return -1; - vm->def->cputune.shares = val; + if (vm->def->cputune.shares != val) { + vm->def->cputune.shares = val; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, "shares", + val) < 0) + return -1; + + event = virDomainEventCputuneNewFromObj(vm, eventParams, eventNparams); + } + + if (event) + qemuDomainEventQueue(vm->privateData, event); }
return 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a8cda43..2389e32 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4462,6 +4462,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virBitmapPtr pcpumap = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4569,6 +4575,18 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + if (virSnprintf(paramField, "vcpu%d", vcpu) < 0) {
As pointed out for 1/5 - this really could just be an snprintf()
+ goto cleanup; + }
The extra {} ^^^ and vvv are probably not necessary
+ + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) { + goto cleanup; + } + + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -4604,6 +4622,9 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virCgroupFree(&cgroup_vcpu); if (vm) virObjectUnlock(vm); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); virBitmapFree(pcpumap); virObjectUnref(caps); virObjectUnref(cfg); @@ -4728,6 +4749,12 @@ qemuDomainPinEmulator(virDomainPtr dom, virBitmapPtr pcpumap = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + virObjectEventPtr event = NULL; + char * str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; +
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4833,6 +4860,14 @@ qemuDomainPinEmulator(virDomainPtr dom,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, "emulatorpin", str) < 0) { + goto cleanup; + }
ditto on the {}
+ + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -4862,6 +4897,9 @@ qemuDomainPinEmulator(virDomainPtr dom, cleanup: if (cgroup_emulator) virCgroupFree(&cgroup_emulator); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); virBitmapFree(pcpumap); virObjectUnref(caps); if (vm) @@ -9054,6 +9092,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxNparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -9124,6 +9166,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "shares", + val) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -9141,6 +9188,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "period", + value_ul) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9155,6 +9207,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.quota = value_l; + + if (virTypedParamsAddLLong(&eventParams, &eventNparams, + &eventMaxNparams, "quota", + value_l) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9170,6 +9227,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.emulator_period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "emulator_period", + value_ul) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9185,6 +9247,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.emulator_quota = value_l; + + if (virTypedParamsAddLLong(&eventParams, &eventNparams, + &eventMaxNparams, "emulator_quota", + value_l) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vmdef = NULL; }
+
Extraneous line ACK with the adjustments mentioned. I think you should just skip the virSnprintf for this series and add one in another series to encompass all callers (and of course add the 'rule' like Eric suggests in 1/5 John
ret = 0;
cleanup: virDomainDefFree(vmdef); if (vm) virObjectUnlock(vm); + if (eventNparams) { + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams); + if (event) + qemuDomainEventQueue(driver, event); + } virObjectUnref(caps); virObjectUnref(cfg); return ret;
On 09/12/2014 10:31 PM, John Ferlan wrote:
s//"something to describe what's being done!"
Might be nice to see what the event would look like when it's triggered. May help in that documentation effort in the future.
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 18 ++++++++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-)
So this is only going to occur as asked in 4/4 in v1 when/if the cputune.shares value is adjusted during qemuProcessStart.
Yes, that's right.
Also events are only triggered when someone uses the api that virsh vcpupin, emulatorpin, and schedinfo(set) call. Which is "ok' by me for the onlist iothreadpin changes which don't yet have a similar iothreadpin api. Meaning my other series shouldn't be affected
[..]
@@ -4569,6 +4575,18 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + if (virSnprintf(paramField, "vcpu%d", vcpu) < 0) {
As pointed out for 1/5 - this really could just be an snprintf()
+ goto cleanup; + }
The extra {} ^^^ and vvv are probably not necessary
[..]
@@ -4833,6 +4860,14 @@ qemuDomainPinEmulator(virDomainPtr dom,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, "emulatorpin", str) < 0) { + goto cleanup; + }
ditto on the {}
[..]
@@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vmdef = NULL; }
+
Extraneous line
ACK with the adjustments mentioned. I think you should just skip the virSnprintf for this series and add one in another series to encompass all callers (and of course add the 'rule' like Eric suggests in 1/5
Will fix the brackets and the extra line, thanks. Pavel
John
On 09/10/2014 06:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 18 ++++++++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-)
@@ -676,6 +677,10 @@ static int qemuSetupCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams; + int eventNparams = 0; + int eventMaxparams = 0;
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { if (vm->def->cputune.sharesSpecified) { @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) return -1; - vm->def->cputune.shares = val; + if (vm->def->cputune.shares != val) { + vm->def->cputune.shares = val; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, "shares", + val) < 0) + return -1; + + event = virDomainEventCputuneNewFromObj(vm, eventParams, eventNparams); + } + + if (event) + qemuDomainEventQueue(vm->privateData, event); }
Continuing my comments from 3/5: Memory leak - if virDomainEventCputuneNewFromObj() fails, nothing frees eventParams; but since this particular event was exactly one typed parameter, at least you have no other problems. Similar leaks for other single-element lists. But then we have this:
@@ -9054,6 +9092,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxNparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -9124,6 +9166,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "shares", + val) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -9141,6 +9188,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "period", + value_ul) < 0) + goto cleanup; }
Hmm. Now you have an event with more than one list member. Suppose that you successfully add "shares" to the list, then run out of memory on virTypedParamsAddULLong() for "period". What happens to the partially-allocated list? ...
@@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vmdef = NULL; }
+ ret = 0;
cleanup: virDomainDefFree(vmdef); if (vm) virObjectUnlock(vm); + if (eventNparams) { + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams);
...oh. This says you are attempting to create the event anyways, in spite of having hit OOM on the attempt to add the second list member. So I suppose that if you do transfer semantics, even the partial list will eventually get cleaned up by the attempt to create the event. But the fact that you failed to create a full list makes it all the more likely that you will also fail to create the event, and probably have a leak on your hands. Is it even worth trying to create the event if the reason that we jumped to cleanup was due to an OOM condition in populating the list of parameters the event would include? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/2014 12:12 AM, Eric Blake wrote:
On 09/10/2014 06:08 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 18 ++++++++++++- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-)
@@ -676,6 +677,10 @@ static int qemuSetupCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams; + int eventNparams = 0; + int eventMaxparams = 0;
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { if (vm->def->cputune.sharesSpecified) { @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) return -1; - vm->def->cputune.shares = val; + if (vm->def->cputune.shares != val) { + vm->def->cputune.shares = val; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, "shares", + val) < 0) + return -1; + + event = virDomainEventCputuneNewFromObj(vm, eventParams, eventNparams); + } + + if (event) + qemuDomainEventQueue(vm->privateData, event); }
Continuing my comments from 3/5: Memory leak - if virDomainEventCputuneNewFromObj() fails, nothing frees eventParams; but since this particular event was exactly one typed parameter, at least you have no other problems. Similar leaks for other single-element lists.
But then we have this:
@@ -9054,6 +9092,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virObjectEventPtr event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxNparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -9124,6 +9166,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "shares", + val) < 0) + goto cleanup; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -9141,6 +9188,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup;
vm->def->cputune.period = value_ul; + + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxNparams, "period", + value_ul) < 0) + goto cleanup; }
Hmm. Now you have an event with more than one list member. Suppose that you successfully add "shares" to the list, then run out of memory on virTypedParamsAddULLong() for "period". What happens to the partially-allocated list? ...
@@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, vmdef = NULL; }
+ ret = 0;
cleanup: virDomainDefFree(vmdef); if (vm) virObjectUnlock(vm); + if (eventNparams) { + event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams);
...oh. This says you are attempting to create the event anyways, in spite of having hit OOM on the attempt to add the second list member. So I suppose that if you do transfer semantics, even the partial list will eventually get cleaned up by the attempt to create the event. But the fact that you failed to create a full list makes it all the more likely that you will also fail to create the event, and probably have a leak on your hands. Is it even worth trying to create the event if the reason that we jumped to cleanup was due to an OOM condition in populating the list of parameters the event would include?
I'll change the behavior to trigger the event only when the live update succeed and for other case I'll free the eventParams. Thanks, Pavel
participants (3)
-
Eric Blake -
John Ferlan -
Pavel Hrdina