[libvirt] [PATCHv2 0/5] Bulk domain stats query API
This version adds implementation in the qemu driver and virsh and fixes the following bugs from v1: *bad type of typed param count *off-by-one when counting and verifying domain objects in libvirt.c *off-by-one when allocating returned array in remote driver *the all domain stats func not working as the return value wasn't handled properly Peter Krempa (5): lib: Introduce API for retrieving bulk domain stats remote: Implement bulk domain stats APIs in the remote driver qemu: Implement bulk stats API and one of the stats groups to return virsh: domain: Split out code to lookup domain from string virsh: Implement command to excercise the bulk stats APIs daemon/remote.c | 91 ++++++++++++++++++++++ include/libvirt/libvirt.h.in | 26 +++++++ src/driver.h | 9 +++ src/libvirt.c | 180 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 ++ src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 88 +++++++++++++++++++++ src/remote/remote_protocol.x | 26 ++++++- src/remote_protocol-structs | 23 ++++++ tools/virsh-domain-monitor.c | 140 +++++++++++++++++++++++++++++++++ tools/virsh-domain.c | 80 ++++++++++++------- tools/virsh-domain.h | 4 + 12 files changed, 819 insertions(+), 30 deletions(-) -- 2.0.2
The motivation for this API is that management layers that use libvirt usually poll for statistics using various split up APIs we currently provide. To get all the necessary stuff, the app needs to issue a lot of calls and agregate the results. The APIs I'm introducing here: 1) Returns data in a format that we can expand in the future and is (pseudo) hierarchical. The data is returned as typed parameters where the fields are constructed as dot-separated strings containing names and other stuff in a list of typed params. 2) Stats for multiple (all) domains can be queried at once and are returned in one call. This will allow to decrease the overhead necessary to issue multiple calls per domain multiplied by the count of domains. 3) Selectable (bit mask) fields in the returned format. This will allow to retrieve only specific stats according to the apps need. The stats groups will be enabled using a bit field @stats passed as the function argument. A few sample stats groups that this API will support: VIR_DOMAIN_STATS_STATE VIR_DOMAIN_STATS_CPU VIR_DOMAIN_STATS_BLOCK VIR_DOMAIN_STATS_INTERFACE the returned typed params will use the following scheme state.state = running state.reason = started cpu.count = 8 cpu.0.state = running cpu.0.time = 1234 --- include/libvirt/libvirt.h.in | 26 +++++++ src/driver.h | 9 +++ src/libvirt.c | 180 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 ++ 4 files changed, 222 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 47ea695..94eba86 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2501,6 +2501,32 @@ int virDomainDetachDeviceFlags(virDomainPtr domain, int virDomainUpdateDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags); +typedef struct _virDomainStatsRecord virDomainStatsRecord; +typedef virDomainStatsRecord *virDomainStatsRecordPtr; +struct _virDomainStatsRecord { + virDomainPtr dom; + int nparams; + virTypedParameterPtr params; +}; + +typedef enum { + VIR_DOMAIN_STATS_ALL = (1 << 0), /* return all stats fields + implemented in the daemon */ + VIR_DOMAIN_STATS_STATE = (1 << 1), /* return domain state */ +} virDomainStatsTypes; + +int virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + +int virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + +void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); + /* * BlockJob API */ diff --git a/src/driver.h b/src/driver.h index ba7c1fc..d5596ab 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1191,6 +1191,14 @@ typedef int unsigned int flags); +typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr; @@ -1411,6 +1419,7 @@ struct _virDriver { virDrvDomainSetTime domainSetTime; virDrvNodeGetFreePages nodeGetFreePages; virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; + virDrvDomainListGetStats domainListGetStats; }; diff --git a/src/libvirt.c b/src/libvirt.c index 8349261..4ec2761 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21341,3 +21341,183 @@ virConnectGetDomainCapabilities(virConnectPtr conn, virDispatchError(conn); return NULL; } + + +/** + * virConnectGetAllDomainStats: + * @conn: pointer to the hypervisor connection + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for all domains on a given connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each storage driver): + * + * VIR_DOMAIN_STATS_ALL: Return all statistics supported by the hypervisor + * driver. This allows to query everything the driver supports without getting + * an error. + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from + * virDomain*Reason enmum corresponding to given state. + * + * Returns the count of returned statistics strucutres on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, stats=0x%x, retStats=%p, flags=0x%x", + conn, stats, retStats, flags); + + virResetLastError(); + + virCheckConnectReturn(conn, -1); + + if (!conn->driver->domainListGetStats) { + virReportUnsupportedError(); + goto cleanup; + } + + ret = conn->driver->domainListGetStats(conn, NULL, 0, stats, + retStats, flags); + + cleanup: + if (ret < 0) + virDispatchError(conn); + + return ret; +} + + +/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for domains provided by @doms. Note that all domains in + * @doms must share the same connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each storage driver): + * + * VIR_DOMAIN_STATS_ALL: Return all statistics supported by the hypervisor + * driver. This allows to query everything the driver supports without getting + * an error. + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from + * virDomain*Reason enmum corresponding to given state. + * + * Returns the count of returned statistics strucutres on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virConnectPtr conn = NULL; + virDomainPtr *nextdom = doms; + unsigned int ndoms = 0; + int ret = -1; + + VIR_DEBUG("doms=%p, stats=0x%x, retStats=%p, flags=0x%x", + doms, stats, retStats, flags); + + virResetLastError(); + + if (!*doms) { + virReportError(VIR_ERR_INVALID_ARG, + _("doms array in %s must contain at least one domain"), + __FUNCTION__); + goto cleanup; + } + + conn = doms[0]->conn; + virCheckConnectReturn(conn, -1); + + if (!conn->driver->domainListGetStats) { + virReportUnsupportedError(); + goto cleanup; + } + + while (*nextdom) { + virDomainPtr dom = *nextdom; + + virCheckDomainGoto(dom, cleanup); + + if (dom->conn != conn) { + virReportError(VIR_ERR_INVALID_ARG, + _("domains in 'doms' array must belong to a " + "single connection in %s"), __FUNCTION__); + goto cleanup; + } + + ndoms++; + nextdom++; + } + + ret = conn->driver->domainListGetStats(conn, doms, ndoms, stats, retStats, + flags); + + cleanup: + if (ret < 0) + virDispatchError(conn); + return ret; +} + + +/** + * virDomainStatsRecordListFree: + * @stats: NULL terminated array of virDomainStatsRecords to free + * + * Convenience function to free a list of domain stats returned by + * virDomainListGetStats and virConnectGetAllDomainStats. + */ +void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next = stats; + + while (*next) { + virTypedParamsFree((*next)->params, (*next)->nparams); + virDomainFree((*next)->dom); + VIR_FREE((*next)); + next++; + } + + VIR_FREE(stats); +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 9f4016a..3714159 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -670,4 +670,11 @@ LIBVIRT_1.2.7 { virConnectGetDomainCapabilities; } LIBVIRT_1.2.6; +LIBVIRT_1.2.8 { + global: + virDomainListGetStats; + virConnectGetAllDomainStats; + virDomainStatsRecordListFree; +} LIBVIRT_1.2.7; + # .... define new API here using predicted next version number .... -- 2.0.2
On 08/26/2014 08:14 AM, Peter Krempa wrote:
The motivation for this API is that management layers that use libvirt usually poll for statistics using various split up APIs we currently provide. To get all the necessary stuff, the app needs to issue a lot of calls and agregate the results.
s/agregate/aggregate/
The APIs I'm introducing here: 1) Returns data in a format that we can expand in the future and is (pseudo) hierarchical. The data is returned as typed parameters where the fields are constructed as dot-separated strings containing names and other stuff in a list of typed params.
2) Stats for multiple (all) domains can be queried at once and are returned in one call. This will allow to decrease the overhead necessary to issue multiple calls per domain multiplied by the count of domains.
3) Selectable (bit mask) fields in the returned format. This will allow to retrieve only specific stats according to the apps need.
The stats groups will be enabled using a bit field @stats passed as the function argument. A few sample stats groups that this API will support:
VIR_DOMAIN_STATS_STATE VIR_DOMAIN_STATS_CPU VIR_DOMAIN_STATS_BLOCK VIR_DOMAIN_STATS_INTERFACE
the returned typed params will use the following scheme
state.state = running state.reason = started cpu.count = 8 cpu.0.state = running cpu.0.time = 1234
Not in this patch, but based on that description, I'd expect something like: block.count = 2 block.0.dev = vda block.0.rd_req = 123 block.0.rd_bytes = 10000 ... block.1.dev = vdb It's been through a few rounds of back and forth on design, but this sounds reasonable to commit to.
--- include/libvirt/libvirt.h.in | 26 +++++++ src/driver.h | 9 +++ src/libvirt.c | 180 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 ++ 4 files changed, 222 insertions(+)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 47ea695..94eba86 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2501,6 +2501,32 @@ int virDomainDetachDeviceFlags(virDomainPtr domain, int virDomainUpdateDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags);
+typedef struct _virDomainStatsRecord virDomainStatsRecord; +typedef virDomainStatsRecord *virDomainStatsRecordPtr; +struct _virDomainStatsRecord { + virDomainPtr dom; + int nparams; + virTypedParameterPtr params; +};
Too bad we already have the precedence of 'int nparams' instead of 'size_t nparams', but at least it is consistent, and unlikely to overflow. All our functions list params/nparams, but this struct lists in reverse order. It shouldn't hurt to swap the order for consistency.
+ +typedef enum { + VIR_DOMAIN_STATS_ALL = (1 << 0), /* return all stats fields + implemented in the daemon */ + VIR_DOMAIN_STATS_STATE = (1 << 1), /* return domain state */ +} virDomainStatsTypes;
This is fewer stats groups than what the commit message. Adding more in later patches is okay, and can happen post-freeze, if the API makes it in pre-freeze, but you might want to make it clear in the commit message that you have saved other stats for later patches.
+ +int virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags);
Hmmm. If I understand right, stats is a bitmask, and you error out if a bit is requested that the daemon doesn't support. On the other hand, VIR_DOMAIN_STATS_ALL is not a mask representing all other bits, but more of a flag that says "give me all available stats without regards to the bitmask". Would it be better to have the following semantics: If stats is 0, provide all possible stats. If stats is non-zero, it is the bitmask of stats that MUST be provided, where VIR_DOMAIN_STATS_STATE = 1, VIR_DOMAIN_STATS_CPU = 2, VIR_DOMAIN_STATS_BLOCK = 4. For convenience, the value VIR_DOMAIN_STATS_ALL is a bitmask of all the other bits (with these three categories, it would be 7) Then, in the flags argument, a flags of 0 says to ignore bitmask members that cannot be provided, while a flags of VIR_DOMAIN_STATS_ENFORCE says to fail if a bitmask is provided but that stat is not available (and of course, makes no difference if the stats bitmask was 0). Thus, these three are the same: virConnectGetAllDomainStats(conn, 0, &array, 0) virConnectGetAllDomainStats(conn, 0, &array, VIR_DOMAIN_STATS_ENFORCE) virConnectGetAllDomainStats(conn, VIR_DOMAIN_STATS_ALL, &array, 0) while this one guarantees that ALL stats are provided, or that the API will outright fail: virConnectGetAllDomainStats(conn, VIR_DOMAIN_STATS_ALL, &array, VIR_DOMAIN_STATS_ENFORCE)
+ +int virDomainListGetStats(virDomainPtr *doms,
I take it *doms is a NULL-terminated array? (I guess I'll get to the docs later in the patch).
+ unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + +void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); + /* * BlockJob API */ diff --git a/src/driver.h b/src/driver.h index ba7c1fc..d5596ab 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1191,6 +1191,14 @@ typedef int unsigned int flags);
+typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags);
Ah, so under the hood, we only have to implement one callback, where doms is NULL for virConnectGetAllDomainStats and non-NULL for virDomainListGetStats? I suppose that works, although it might make the RPC interesting. I guess I'll see later in the series, but as this is internal-only, it shouldn't matter for what we commit to in the public API.
+ + +/** + * virConnectGetAllDomainStats: + * @conn: pointer to the hypervisor connection + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0
Would it make sense for @flags to provide the same filtering as virConnectListAllDomains(), for selecting a subset of domains to get stats on? But can definitely be added later.
+ * + * Query statistics for all domains on a given connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each storage driver):
s/storage driver/hypervisor/
+ * + * VIR_DOMAIN_STATS_ALL: Return all statistics supported by the hypervisor + * driver. This allows to query everything the driver supports without getting + * an error.
See my above comments about how this feels like it should be something in the @flags parameter, rather than the @stats parameter (that is, a flag _ENFORCE that says whether the hypervisor lacking a particular stat group is fatal).
+ * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from
s/in/int/
+ * virDomain*Reason enmum corresponding to given state.
s/enmum/enum/
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
s/strucutres/structures/
+ * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, stats=0x%x, retStats=%p, flags=0x%x", + conn, stats, retStats, flags); + + virResetLastError(); + + virCheckConnectReturn(conn, -1);
virCheckNonNullArg(retStats)? Matters depending on whether you plan to allow NULL retStats across RPC. Right now, it looks like you are planning on allowing NULL for retStats to use the return value to see how many domains there are (or get an error that a requested stat is impossible). Is it worth that complexity, given that virConnectListAllDomains can already return a count of the number of domains? Then again, consistency is easier to copy-and-paste and therefore maintain.
+/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains
I'm a little bit surprised that we aren't requiring the user to just pass a length; but since virConnectListAllDomains returns a NULL-terminated array, having one less parameter does seem easier on the caller.
+ * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for domains provided by @doms. Note that all domains in + * @doms must share the same connection.
Yes, that better be enforced :)
+ * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from + * virDomain*Reason enmum corresponding to given state.
Same typos as above. I'm okay with the duplication (it's easier for a user to read the right docs on the first hit than to have to chase links), but be sure to fix both places identically.
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
Probably worth a simple mention that the return count may be smaller than the count of doms passed in. (Probably not worth mentioning why, but possible reasons might include: domain no longer exists, duplicate domains on input consolidated into common domain on output, driver can only list stats for a running domain so it omits an entire structure for an offline domain, ...)
+int +virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virConnectPtr conn = NULL; + virDomainPtr *nextdom = doms; + unsigned int ndoms = 0; + int ret = -1; + + VIR_DEBUG("doms=%p, stats=0x%x, retStats=%p, flags=0x%x", + doms, stats, retStats, flags); + + virResetLastError(); + + if (!*doms) {
Missing virCheckNonNullArgReturn(doms, -1) prior to dereferencing *doms. (I would have suggested virCheckNonNullArgGoto(doms, error), except that the error label only works if we have validated the connection).
+ virReportError(VIR_ERR_INVALID_ARG, + _("doms array in %s must contain at least one domain"), + __FUNCTION__); + goto cleanup;
Ouch. You can't use the cleanup label unless you know the conn is valid, but you don't validate the conn until...
+ } + + conn = doms[0]->conn; + virCheckConnectReturn(conn, -1);
...here. All failures before this point have to be direct 'return -1'. But you want as few of those as possible, so that the bulk of the error detection can report the error through the connection.
+void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next = stats; + + while (*next) { + virTypedParamsFree((*next)->params, (*next)->nparams); + virDomainFree((*next)->dom); + VIR_FREE((*next));
Extra () on that last line (only needed on the first 2 of the three lines). Looking close, but I'd feel more comfortable with a v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/14 18:49, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
....
+typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags);
Ah, so under the hood, we only have to implement one callback, where doms is NULL for virConnectGetAllDomainStats and non-NULL for virDomainListGetStats? I suppose that works, although it might make the RPC interesting. I guess I'll see later in the series, but as this is internal-only, it shouldn't matter for what we commit to in the public API.
+ + +/** + * virConnectGetAllDomainStats: + * @conn: pointer to the hypervisor connection + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0
Would it make sense for @flags to provide the same filtering as virConnectListAllDomains(), for selecting a subset of domains to get stats on? But can definitely be added later.
I already thought about implementing the filtering capability for this function although I doubt that all of the filtering flags may be useful here. Requesting stats for a machine that has (or doesn't have) snapshot doesn't seem useful at all.
+ * + * Query statistics for all domains on a given connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each storage driver):
s/storage driver/hypervisor/
+ * + * VIR_DOMAIN_STATS_ALL: Return all statistics supported by the hypervisor + * driver. This allows to query everything the driver supports without getting + * an error.
See my above comments about how this feels like it should be something in the @flags parameter, rather than the @stats parameter (that is, a flag _ENFORCE that says whether the hypervisor lacking a particular stat group is fatal).
+ * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from
s/in/int/
+ * virDomain*Reason enmum corresponding to given state.
s/enmum/enum/
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
s/strucutres/structures/
+ * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, stats=0x%x, retStats=%p, flags=0x%x", + conn, stats, retStats, flags); + + virResetLastError(); + + virCheckConnectReturn(conn, -1);
virCheckNonNullArg(retStats)? Matters depending on whether you plan to allow NULL retStats across RPC.
Yep we always want to have retStats. I forgot to add the check.
Right now, it looks like you are planning on allowing NULL for retStats to use the return value to see how many domains there are (or get an error that a requested stat is impossible). Is it worth that complexity, given that virConnectListAllDomains can already return a count of the number of domains? Then again, consistency is easier to copy-and-paste and therefore maintain.
+/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains
I'm a little bit surprised that we aren't requiring the user to just pass a length; but since virConnectListAllDomains returns a NULL-terminated array, having one less parameter does seem easier on the caller.
+ * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for domains provided by @doms. Note that all domains in + * @doms must share the same connection.
Yes, that better be enforced :)
+ * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as in from + * virDomain*Reason enmum corresponding to given state.
Same typos as above. I'm okay with the duplication (it's easier for a user to read the right docs on the first hit than to have to chase links), but be sure to fix both places identically.
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
Probably worth a simple mention that the return count may be smaller than the count of doms passed in. (Probably not worth mentioning why, but possible reasons might include: domain no longer exists, duplicate domains on input consolidated into common domain on output, driver can only list stats for a running domain so it omits an entire structure for an offline domain, ...)
+int +virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virConnectPtr conn = NULL; + virDomainPtr *nextdom = doms; + unsigned int ndoms = 0; + int ret = -1; + + VIR_DEBUG("doms=%p, stats=0x%x, retStats=%p, flags=0x%x", + doms, stats, retStats, flags); + + virResetLastError(); + + if (!*doms) {
Missing virCheckNonNullArgReturn(doms, -1) prior to dereferencing *doms. (I would have suggested virCheckNonNullArgGoto(doms, error), except that the error label only works if we have validated the connection).
+ virReportError(VIR_ERR_INVALID_ARG, + _("doms array in %s must contain at least one domain"), + __FUNCTION__); + goto cleanup;
Ouch. You can't use the cleanup label unless you know the conn is valid, but you don't validate the conn until...
Actually you can. The function that validates "conn" calls the same error dispatching function with NULL argument. Exactly what will happen here.
+ } + + conn = doms[0]->conn; + virCheckConnectReturn(conn, -1);
...here. All failures before this point have to be direct 'return -1'. But you want as few of those as possible, so that the bulk of the error detection can report the error through the connection.
# define virCheckConnectReturn(obj, retval) do { if (!virObjectIsClass(obj, virConnectClass)) { virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INVALID_CONN, __FILE__, __FUNCTION__, __LINE__, __FUNCTION__); virDispatchError(NULL); return retval; } } while (0)
+void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next = stats; + + while (*next) { + virTypedParamsFree((*next)->params, (*next)->nparams); + virDomainFree((*next)->dom); + VIR_FREE((*next));
Extra () on that last line (only needed on the first 2 of the three lines).
Looking close, but I'd feel more comfortable with a v3.
Peter
On 08/26/2014 11:28 AM, Peter Krempa wrote:
On 08/26/14 18:49, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
....
Would it make sense for @flags to provide the same filtering as virConnectListAllDomains(), for selecting a subset of domains to get stats on? But can definitely be added later.
I already thought about implementing the filtering capability for this function although I doubt that all of the filtering flags may be useful here.
Requesting stats for a machine that has (or doesn't have) snapshot doesn't seem useful at all.
True, not all of the listing filters make as much sense here. At any rate, adding filtering as a later extension is a fine approach; and we can always play it by ear according to demand.
virCheckNonNullArg(retStats)? Matters depending on whether you plan to allow NULL retStats across RPC.
Yep we always want to have retStats. I forgot to add the check.
Okay, I did my review of 2/5 before seeing this. But there were still some things to clean up there as a result of enforcing this requirement.
+ virResetLastError(); + + if (!*doms) {
Missing virCheckNonNullArgReturn(doms, -1) prior to dereferencing *doms. (I would have suggested virCheckNonNullArgGoto(doms, error), except that the error label only works if we have validated the connection).
+ virReportError(VIR_ERR_INVALID_ARG, + _("doms array in %s must contain at least one domain"), + __FUNCTION__); + goto cleanup;
Ouch. You can't use the cleanup label unless you know the conn is valid, but you don't validate the conn until...
Actually you can. The function that validates "conn" calls the same error dispatching function with NULL argument. Exactly what will happen here.
Ah, I was getting confused with our typical uses, which look somewhat like: cleanup: if (ret < 0) virDispatchError(dom->conn); where we CANNOT dereference dom if it didn't validate; but in your case, you made sure the local 'conn' is either NULL or grabbed from a validated dom, and virDispatchError(NULL) does work. Okay, false negative on my side. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The motivation for this API is that management layers that use libvirt usually poll for statistics using various split up APIs we currently provide. To get all the necessary stuff, the app needs to issue a lot of calls and aggregate the results. The APIs I'm introducing here: 1) Returns data in a format that we can expand in the future and is (pseudo) hierarchical. The data is returned as typed parameters where the fields are constructed as dot-separated strings containing names and other stuff in a list of typed params. 2) Stats for multiple (all) domains can be queried at once and are returned in one call. This will allow to decrease the overhead necessary to issue multiple calls per domain multiplied by the count of domains. 3) Selectable (bit mask) fields in the returned format. This will allow to retrieve only specific stats according to the apps need. The stats groups will be enabled using a bit field @stats passed as the function argument. A few sample stats groups that this API will support: VIR_DOMAIN_STATS_STATE VIR_DOMAIN_STATS_CPU VIR_DOMAIN_STATS_BLOCK VIR_DOMAIN_STATS_INTERFACE (Note that this is only an example, the initial implementation supports only VIR_DOMAIN_STATS_STATE while others will be added later) the returned typed params will use the following scheme state.state = running state.reason = started cpu.count = 8 cpu.0.state = running cpu.0.time = 1234 --- include/libvirt/libvirt.h.in | 24 ++++++ src/driver.h | 9 +++ src/libvirt.c | 182 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 +- 4 files changed, 220 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 153b386..d5b617e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2501,6 +2501,30 @@ int virDomainDetachDeviceFlags(virDomainPtr domain, int virDomainUpdateDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags); +typedef struct _virDomainStatsRecord virDomainStatsRecord; +typedef virDomainStatsRecord *virDomainStatsRecordPtr; +struct _virDomainStatsRecord { + virDomainPtr dom; + virTypedParameterPtr params; + int nparams; +}; + +typedef enum { + VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ +} virDomainStatsTypes; + +int virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + +int virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + +void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); + /* * BlockJob API */ diff --git a/src/driver.h b/src/driver.h index 39bf219..4e4cb52 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1197,6 +1197,14 @@ typedef int unsigned int flags); +typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags); + typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr; @@ -1418,6 +1426,7 @@ struct _virDriver { virDrvDomainSetTime domainSetTime; virDrvNodeGetFreePages nodeGetFreePages; virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; + virDrvDomainListGetStats domainListGetStats; }; diff --git a/src/libvirt.c b/src/libvirt.c index 8fd0e46..14592d8 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21399,3 +21399,185 @@ virConnectGetDomainCapabilities(virConnectPtr conn, virDispatchError(conn); return NULL; } + + +/** + * virConnectGetAllDomainStats: + * @conn: pointer to the hypervisor connection + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for all domains on a given connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each hypervisor): + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as int from + * virDomain*Reason enum corresponding to given state. + * + * Using 0 for @stats returns all stats groups supported by given hypervisor. + * + * Returns the count of returned statistics strucutres on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, stats=0x%x, retStats=%p, flags=0x%x", + conn, stats, retStats, flags); + + virResetLastError(); + + virCheckConnectReturn(conn, -1); + virCheckNonNullArgGoto(retStats, cleanup); + + if (!conn->driver->domainListGetStats) { + virReportUnsupportedError(); + goto cleanup; + } + + ret = conn->driver->domainListGetStats(conn, NULL, 0, stats, + retStats, flags); + + cleanup: + if (ret < 0) + virDispatchError(conn); + + return ret; +} + + +/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Query statistics for domains provided by @doms. Note that all domains in + * @doms must share the same connection. + * + * Report statistics of various parameters for a running VM according to @stats + * field. The statistics are returned as an array of structures for each queried + * domain. The structure contains an array of typed parameters containing the + * individual statistics. The typed parameter name for each statistic field + * consists of a dot-separated string containing name of the requested group + * followed by a group specific description of the statistic value. + * + * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each hypervisor): + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format: + * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as int from + * virDomain*Reason enum corresponding to given state. + * + * Using 0 for @stats returns all stats groups supported by given hypervisor. + * + * Returns the count of returned statistics structures on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + * + * Note that the count of returned stats may be less than the domain count provided + * via @doms. + */ +int +virDomainListGetStats(virDomainPtr *doms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virConnectPtr conn = NULL; + virDomainPtr *nextdom = doms; + unsigned int ndoms = 0; + int ret = -1; + + VIR_DEBUG("doms=%p, stats=0x%x, retStats=%p, flags=0x%x", + doms, stats, retStats, flags); + + virResetLastError(); + + virCheckNonNullArgGoto(doms, cleanup); + virCheckNonNullArgGoto(retStats, cleanup); + + if (!*doms) { + virReportError(VIR_ERR_INVALID_ARG, + _("doms array in %s must contain at least one domain"), + __FUNCTION__); + goto cleanup; + } + + conn = doms[0]->conn; + virCheckConnectReturn(conn, -1); + + if (!conn->driver->domainListGetStats) { + virReportUnsupportedError(); + goto cleanup; + } + + while (*nextdom) { + virDomainPtr dom = *nextdom; + + virCheckDomainGoto(dom, cleanup); + + if (dom->conn != conn) { + virReportError(VIR_ERR_INVALID_ARG, + _("domains in 'doms' array must belong to a " + "single connection in %s"), __FUNCTION__); + goto cleanup; + } + + ndoms++; + nextdom++; + } + + ret = conn->driver->domainListGetStats(conn, doms, ndoms, stats, retStats, + flags); + + cleanup: + if (ret < 0) + virDispatchError(conn); + return ret; +} + + +/** + * virDomainStatsRecordListFree: + * @stats: NULL terminated array of virDomainStatsRecords to free + * + * Convenience function to free a list of domain stats returned by + * virDomainListGetStats and virConnectGetAllDomainStats. + */ +void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next; + + for (next = stats; *next; next++) { + virTypedParamsFree((*next)->params, (*next)->nparams); + virDomainFree((*next)->dom); + VIR_FREE(*next); + } + + VIR_FREE(stats); +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index ce5aeeb..866957c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -671,8 +671,11 @@ LIBVIRT_1.2.7 { } LIBVIRT_1.2.6; LIBVIRT_1.2.8 { - global: - virDomainOpenGraphicsFD; + global: + virDomainOpenGraphicsFD; + virDomainListGetStats; + virConnectGetAllDomainStats; + virDomainStatsRecordListFree; } LIBVIRT_1.2.7; # .... define new API here using predicted next version number .... -- 2.0.2
On 08/26/2014 01:11 PM, Peter Krempa wrote:
The motivation for this API is that management layers that use libvirt usually poll for statistics using various split up APIs we currently provide. To get all the necessary stuff, the app needs to issue a lot of calls and aggregate the results.
The APIs I'm introducing here: 1) Returns data in a format that we can expand in the future and is (pseudo) hierarchical. The data is returned as typed parameters where the fields are constructed as dot-separated strings containing names and other stuff in a list of typed params.
2) Stats for multiple (all) domains can be queried at once and are returned in one call. This will allow to decrease the overhead necessary
s/allow to //
to issue multiple calls per domain multiplied by the count of domains.
3) Selectable (bit mask) fields in the returned format. This will allow to retrieve only specific stats according to the apps need.
s/apps/app's/
The stats groups will be enabled using a bit field @stats passed as the function argument. A few sample stats groups that this API will support:
VIR_DOMAIN_STATS_STATE VIR_DOMAIN_STATS_CPU VIR_DOMAIN_STATS_BLOCK VIR_DOMAIN_STATS_INTERFACE
(Note that this is only an example, the initial implementation supports only VIR_DOMAIN_STATS_STATE while others will be added later)
the returned typed params will use the following scheme
state.state = running state.reason = started
Actually, you return int enum values, rather than a stringized state name. So this would better be listed as state.state = VIR_DOMAIN_RUNNING state.reason = VIR_DOMAIN_RUNNING_BOOTED
cpu.count = 8 cpu.0.state = running cpu.0.time = 1234 ---
+++ b/src/driver.h @@ -1197,6 +1197,14 @@ typedef int unsigned int flags);
+typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags);
I'm still worried that this may be generating the wrong ACL functions, and that it should be named virDrvConnectDomainListGetStats so as to give a hint to the generators that this is an operation on the connection, that happens to take an optional DomainList argument.
+ typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr;
@@ -1418,6 +1426,7 @@ struct _virDriver { virDrvDomainSetTime domainSetTime; virDrvNodeGetFreePages nodeGetFreePages; virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; + virDrvDomainListGetStats domainListGetStats;
I'd name this callback member connectDomainListGetStats.
+ * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each hypervisor): + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format:
s/in/in this/
+ * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as int from + * virDomain*Reason enum corresponding to given state. + * + * Using 0 for @stats returns all stats groups supported by given hypervisor.
s/given/the given/
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
s/strucutres/structures/
+ * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{
implementation looks okay
+/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + *
+ * Using 0 for @stats returns all stats groups supported by given hypervisor. + * + * Returns the count of returned statistics structures on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + * + * Note that the count of returned stats may be less than the domain count provided + * via @doms.
The docs generator doesn't like paragraphs that appear after a sentence starting with "Returns"; drop the blank line so that it is part of the returns paragraph and I think you'll be fine.
+ +/** + * virDomainStatsRecordListFree: + * @stats: NULL terminated array of virDomainStatsRecords to free + * + * Convenience function to free a list of domain stats returned by + * virDomainListGetStats and virConnectGetAllDomainStats. + */ +void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next; + + for (next = stats; *next; next++) {
Missing a lead-in: if (!stats) return; which will let users do virDomainStatsRecordListFree(NULL) and ease their cleanup code.
LIBVIRT_1.2.8 { - global: - virDomainOpenGraphicsFD; + global: + virDomainOpenGraphicsFD; + virDomainListGetStats; + virConnectGetAllDomainStats; + virDomainStatsRecordListFree;
I tend to sort these alphabetically, but it doesn't strictly matter. ACK with the comments above addressed. (The biggest impact may be that renaming the driver.h callback name will impact the ACL generator and cause you some headaches as you respin patch 3 in the series) Let's commit the API now, so we can do the release candidate, and the rest of the series can dribble in between now and rc2 as needed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/14 21:41, Eric Blake wrote:
On 08/26/2014 01:11 PM, Peter Krempa wrote:
The motivation for this API is that management layers that use libvirt usually poll for statistics using various split up APIs we currently provide. To get all the necessary stuff, the app needs to issue a lot of calls and aggregate the results.
The APIs I'm introducing here: 1) Returns data in a format that we can expand in the future and is (pseudo) hierarchical. The data is returned as typed parameters where the fields are constructed as dot-separated strings containing names and other stuff in a list of typed params.
2) Stats for multiple (all) domains can be queried at once and are returned in one call. This will allow to decrease the overhead necessary
s/allow to //
to issue multiple calls per domain multiplied by the count of domains.
3) Selectable (bit mask) fields in the returned format. This will allow to retrieve only specific stats according to the apps need.
s/apps/app's/
The stats groups will be enabled using a bit field @stats passed as the function argument. A few sample stats groups that this API will support:
VIR_DOMAIN_STATS_STATE VIR_DOMAIN_STATS_CPU VIR_DOMAIN_STATS_BLOCK VIR_DOMAIN_STATS_INTERFACE
(Note that this is only an example, the initial implementation supports only VIR_DOMAIN_STATS_STATE while others will be added later)
the returned typed params will use the following scheme
state.state = running state.reason = started
Actually, you return int enum values, rather than a stringized state name. So this would better be listed as state.state = VIR_DOMAIN_RUNNING state.reason = VIR_DOMAIN_RUNNING_BOOTED
cpu.count = 8 cpu.0.state = running cpu.0.time = 1234 ---
+++ b/src/driver.h @@ -1197,6 +1197,14 @@ typedef int unsigned int flags);
+typedef int +(*virDrvDomainListGetStats)(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags);
I'm still worried that this may be generating the wrong ACL functions, and that it should be named virDrvConnectDomainListGetStats so as to give a hint to the generators that this is an operation on the connection, that happens to take an optional DomainList argument.
I had to rename it to virDrvConnectGetAllDomainStats to make the API doc generator happy.
+ typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr;
@@ -1418,6 +1426,7 @@ struct _virDriver { virDrvDomainSetTime domainSetTime; virDrvNodeGetFreePages nodeGetFreePages; virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; + virDrvDomainListGetStats domainListGetStats;
I'd name this callback member connectDomainListGetStats.
+ * The statistic groups are enabled using the @stats parameter which is a + * binary-OR of enum virDomainStatsTypes. The following groups are available + * (although not necessarily implemented for each hypervisor): + * + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that + * state. The typed parameter keys are in format:
s/in/in this/
+ * "state.state" - state of the VM, returned as int from virDomainState enum + * "state.reason" - reason for entering given state, returned as int from + * virDomain*Reason enum corresponding to given state. + * + * Using 0 for @stats returns all stats groups supported by given hypervisor.
s/given/the given/
+ * + * Returns the count of returned statistics strucutres on success, -1 on error.
s/strucutres/structures/
+ * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + */ +int +virConnectGetAllDomainStats(virConnectPtr conn, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{
implementation looks okay
+/** + * virDomainListGetStats: + * @doms: NULL terminated array of domains + * @stats: stats to return, binary-OR of virDomainStatsTypes + * @retStats: Pointer that will be filled with the array of returned stats. + * @flags: extra flags; not used yet, so callers should always pass 0 + *
+ * Using 0 for @stats returns all stats groups supported by given hypervisor. + * + * Returns the count of returned statistics structures on success, -1 on error. + * The requested data are returned in the @retStats parameter. The returned + * array should be freed by the caller. See virDomainStatsRecordListFree. + * + * Note that the count of returned stats may be less than the domain count provided + * via @doms.
The docs generator doesn't like paragraphs that appear after a sentence starting with "Returns"; drop the blank line so that it is part of the returns paragraph and I think you'll be fine.
+ +/** + * virDomainStatsRecordListFree: + * @stats: NULL terminated array of virDomainStatsRecords to free + * + * Convenience function to free a list of domain stats returned by + * virDomainListGetStats and virConnectGetAllDomainStats. + */ +void +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats) +{ + virDomainStatsRecordPtr *next; + + for (next = stats; *next; next++) {
Missing a lead-in:
if (!stats) return;
which will let users do virDomainStatsRecordListFree(NULL) and ease their cleanup code.
LIBVIRT_1.2.8 { - global: - virDomainOpenGraphicsFD; + global: + virDomainOpenGraphicsFD; + virDomainListGetStats; + virConnectGetAllDomainStats; + virDomainStatsRecordListFree;
I tend to sort these alphabetically, but it doesn't strictly matter.
ACK with the comments above addressed. (The biggest impact may be that renaming the driver.h callback name will impact the ACL generator and cause you some headaches as you respin patch 3 in the series)
Let's commit the API now, so we can do the release candidate, and the rest of the series can dribble in between now and rc2 as needed.
Pushed; I'll follow up with the rest as soon as possible. Thanks and sorry for messing with the release cycle :/ Peter
Implement the remote driver support for shuffling the domain stats around. --- daemon/remote.c | 91 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 26 ++++++++++++- src/remote_protocol-structs | 23 +++++++++++ 4 files changed, 227 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index ea16789..d488c58 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6337,6 +6337,97 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, } +static int +remoteDispatchDomainListGetStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_list_get_stats_args *args, + remote_domain_list_get_stats_ret *ret) +{ + int rv = -1; + size_t i; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virDomainStatsRecordPtr *retStats = NULL; + int nrecords = 0; + virDomainPtr *doms = NULL; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (args->doms.doms_len) { + if (VIR_ALLOC_N(doms, args->doms.doms_len + 1) < 0) + goto cleanup; + + for (i = 0; i < args->doms.doms_len; i++) { + if (!(doms[i] = get_nonnull_domain(priv->conn, args->doms.doms_val[i]))) + goto cleanup; + } + + if ((nrecords = virDomainListGetStats(doms, + args->stats, + &retStats, + args->flags)) < 0) + goto cleanup; + } else { + if ((nrecords = virConnectGetAllDomainStats(priv->conn, + args->stats, + &retStats, + args->flags)) < 0) + goto cleanup; + } + + if (nrecords > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of domain stats records is %d, " + "which exceeds max limit: %d"), + nrecords, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + + if (retStats && nrecords) { + ret->retStats.retStats_len = nrecords; + + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) + goto cleanup; + + for (i = 0; i < nrecords; i++) { + virDomainStatsRecordPtr src = retStats[i]; + remote_domain_stats_record *dst = ret->retStats.retStats_val + i; + + make_nonnull_domain(&dst->dom, src->dom); + + if (remoteSerializeTypedParameters(src->params, src->nparams, + &dst->params.params_val, + &dst->params.params_len, + VIR_TYPED_PARAM_STRING_OKAY) < 0) + goto cleanup; + } + } else { + ret->retStats.retStats_len = 0; + ret->retStats.retStats_val = NULL; + } + + ret->ret = nrecords; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (retStats) + virDomainStatsRecordListFree(retStats); + if (args->doms.doms_len) { + for (i = 0; i < args->doms.doms_len; i++) + virDomainFree(doms[i]); + + VIR_FREE(doms); + } + return rv; +} + + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9a1d78f..5836745 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7670,6 +7670,93 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net, } +static int +remoteDomainListGetStats(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + struct private_data *priv = conn->networkPrivateData; + int rv = -1; + size_t i; + remote_domain_list_get_stats_args args; + remote_domain_list_get_stats_ret ret; + + virDomainStatsRecordPtr *tmpret = NULL; + + if (ndoms) { + if (VIR_ALLOC_N(args.doms.doms_val, ndoms) < 0) + goto cleanup; + + for (i = 0; i < ndoms; i++) + make_nonnull_domain(args.doms.doms_val + i, doms[i]); + } + args.doms.doms_len = ndoms; + + args.stats = stats; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + remoteDriverLock(priv); + if (call(conn, priv, 0, REMOTE_PROC_DOMAIN_LIST_GET_STATS, + (xdrproc_t)xdr_remote_domain_list_get_stats_args, (char *)&args, + (xdrproc_t)xdr_remote_domain_list_get_stats_ret, (char *)&ret) == -1) { + remoteDriverUnlock(priv); + goto cleanup; + } + remoteDriverUnlock(priv); + + if (ret.retStats.retStats_len > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of stats entries is %d, which exceeds max limit: %d"), + ret.retStats.retStats_len, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + + *retStats = NULL; + + if (ret.retStats.retStats_len) { + if (VIR_ALLOC_N(tmpret, ret.retStats.retStats_len + 1) < 0) + goto cleanup; + + for (i = 0; i < ret.retStats.retStats_len; i++) { + virDomainStatsRecordPtr elem; + remote_domain_stats_record *rec = ret.retStats.retStats_val + i; + + if (VIR_ALLOC(elem) < 0) + goto cleanup; + + if (!(elem->dom = get_nonnull_domain(conn, rec->dom))) + goto cleanup; + + if (remoteDeserializeTypedParameters(rec->params.params_val, + rec->params.params_len, + REMOTE_PROC_DOMAIN_LIST_GET_STATS, + &elem->params, + &elem->nparams)) + goto cleanup; + + tmpret[i] = elem; + } + + *retStats = tmpret; + tmpret = NULL; + } + + rv = ret.ret; + + cleanup: + if (tmpret) + virDomainStatsRecordListFree(tmpret); + xdr_free((xdrproc_t)xdr_remote_domain_list_get_stats_ret, + (char *) &ret); + + return rv; +} + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8008,6 +8095,7 @@ static virDriver remote_driver = { .domainSetTime = remoteDomainSetTime, /* 1.2.5 */ .nodeGetFreePages = remoteNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */ + .domainListGetStats = remoteDomainListGetStats, /* 1.2.8 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5c316fb..9729392 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -241,6 +241,9 @@ const REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX = 256; /* Upper limit on the maximum number of leases in one lease file */ const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536; +/* Upper limit on count of parameters returned via bulk stats API */ +const REMOTE_DOMAIN_STATS_PARAMS_MAX = 2048; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -3057,6 +3060,21 @@ struct remote_network_get_dhcp_leases_ret { unsigned int ret; }; +struct remote_domain_stats_record { + remote_nonnull_domain dom; + remote_typed_param params<REMOTE_DOMAIN_STATS_PARAMS_MAX>; +}; + +struct remote_domain_list_get_stats_args { + remote_nonnull_domain doms<REMOTE_DOMAIN_LIST_MAX>; + unsigned int stats; + unsigned int flags; +}; + +struct remote_domain_list_get_stats_ret { + remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; + int ret; +}; /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -5420,5 +5438,11 @@ enum remote_procedure { * @generate: both * @acl: connect:write */ - REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342 + REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 9bf09b8..3d9f5e7 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2519,6 +2519,28 @@ struct remote_network_get_dhcp_leases_ret { } leases; u_int ret; }; +struct remote_domain_stats_record { + remote_nonnull_domain dom; + struct { + u_int params_len; + remote_typed_param * params_val; + } params; +}; +struct remote_domain_list_get_stats_args { + struct { + u_int doms_len; + remote_nonnull_domain * doms_val; + } doms; + u_int stats; + u_int flags; +}; +struct remote_domain_list_get_stats_ret { + struct { + u_int retStats_len; + remote_domain_stats_record * retStats_val; + } retStats; + int ret; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2862,4 +2884,5 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_FREE_PAGES = 340, REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 341, REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342, + REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343, }; -- 2.0.2
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Implement the remote driver support for shuffling the domain stats around. --- daemon/remote.c | 91 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 26 ++++++++++++- src/remote_protocol-structs | 23 +++++++++++ 4 files changed, 227 insertions(+), 1 deletion(-)
Rearranging things in my review (man, I wish git format-patch -Ofilename could be offloaded into git config by more than just an alias - it's nice to have a filename that lists .x and .h to show before .c).
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5c316fb..9729392 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -241,6 +241,9 @@ const REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX = 256; /* Upper limit on the maximum number of leases in one lease file */ const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536;
+/* Upper limit on count of parameters returned via bulk stats API */ +const REMOTE_DOMAIN_STATS_PARAMS_MAX = 2048;
so up to 2048 stats per domain...
+ /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -3057,6 +3060,21 @@ struct remote_network_get_dhcp_leases_ret { unsigned int ret; };
+struct remote_domain_stats_record { + remote_nonnull_domain dom; + remote_typed_param params<REMOTE_DOMAIN_STATS_PARAMS_MAX>; +}; + +struct remote_domain_list_get_stats_args { + remote_nonnull_domain doms<REMOTE_DOMAIN_LIST_MAX>;
...for up to 16k domains, for as big as a worst-case 32M list of parameters (and each parameter is non-trivial in size). Wow, that can add up to a lot of RPC :) I think your limits are okay, but do worry a little bit that this RPC may bump against limits elsewhere (max RPC size, for starters) if we ever have that many stats per domain. More interesting is how many stats can we return on one domain - a domain with 32 cpus and 32 <disk> elements can generate LOTS of stats. Will we find the 2048 limit too tight?
+ unsigned int stats; + unsigned int flags; +};
Going back to a point I made on 1/5: Your RPC call does NOT special case a user passing a NULL retStats variable, which means the remote side will ALWAYS be populating an array and sending it back, even if the user didn't care about the stats themselves but just the count of structs that would be returned. I'm okay if you require the user to always pass retStats, but if so, enforce that in libvirt.c; if not, then look at remote_connect_list_all_domains_args for what you have to do to signal to the remote side that you are just collecting a count instead of stats, to avoid sending lots of return data just to throw it away.
+ +struct remote_domain_list_get_stats_ret { + remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; + int ret; +};
Looks sufficient to cover both public APIs.
/*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */ @@ -5420,5 +5438,11 @@ enum remote_procedure { * @generate: both * @acl: connect:write */ - REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342 + REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343
Merge conflicts; trivial to resolve :) Did you even try generating the functions, or just assumed it was complex enough to need special handling? At any rate, I'm fine with a manual implementation (I was actually fairly shocked that my virDomainBlockRebase worked with the generated code). So on to that:
diff --git a/daemon/remote.c b/daemon/remote.c index ea16789..d488c58 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6337,6 +6337,97 @@
remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED,
}
+static int +remoteDispatchDomainListGetStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_list_get_stats_args *args, + remote_domain_list_get_stats_ret *ret) +{
+ + if (retStats && nrecords) {
Again, if you are going to special-case the user passing a NULL retStats, then this makes sense; but if you are going to require the user to pass a non-NULL retStats, then it is sufficient to check nrecords being non-negative here, without also having to check for non-NULL retStats.
+ } else { + ret->retStats.retStats_len = 0; + ret->retStats.retStats_val = NULL; + } + + ret->ret = nrecords;
Hmm, ret->ret is redundant if you are going to enforce the user passing non-NULL retStats - just use ret->retStats.retStats_len. The redundancy is necessary only if you are going to support NULL for gathering a count independently from populating the array.
+++ b/src/remote/remote_driver.c @@ -7670,6 +7670,93 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net, }
+static int +remoteDomainListGetStats(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{
+ + if (ret.retStats.retStats_len) { + if (VIR_ALLOC_N(tmpret, ret.retStats.retStats_len + 1) < 0)
Oops. If the return value is 0, you STILL want to alloc a 1-element array with NULL as its only entry, rather than bypassing allocation altogether (unless you are supporting NULL retStats) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/2014 12:22 PM, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Implement the remote driver support for shuffling the domain stats around. --- daemon/remote.c | 91 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 26 ++++++++++++- src/remote_protocol-structs | 23 +++++++++++ 4 files changed, 227 insertions(+), 1 deletion(-)
Rearranging things in my review (man, I wish git format-patch -Ofilename could be offloaded into git config by more than just an alias - it's nice to have a filename that lists .x and .h to show before .c).
+ + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343
Merge conflicts; trivial to resolve :)
Oops, missed a review comment. This is the wrong ACL. I think you want to have: @acl: connect:search_domains @aclfilter: domain:read which says that you can only call this API if you are also allowed to call virConnectListAllDomains (connect:search_domains), and that within this API, you will be filtering any domain that the user cannot normally obtain stats on via other means (for example, virDomainGetState and virDomainGetCPUStats both require domain:read - but they are called on a single domain, whereas this function is called on every domain and filtering out the domains that cannot be called individually). I didn't audit if ALL stats that you plan on querying all use domain:read, or if there are even more limitations to place on this API. Which means patch 3/5 is probably busted for not using a filter function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implement the API function for virDomainListGetStats and virConnectGetAllDomainStats in a modular way and implement the VIR_DOMAIN_STATS_STATE group of statistics. --- src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ad75bd9..7ec2751 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17112,6 +17112,180 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, } +static int +qemuDomainGetStatsState(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED) +{ + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "state.state", + dom->state.state) < 0) + return -1; + + if (virTypedParamsAddInt(&record->params, + &record->nparams, + maxparams, + "state.reason", + dom->state.reason) < 0) + return -1; + + + return 0; +} + + +typedef int +(*virDomainGetStatsFunc)(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags); + +struct virDomainGetStatsWorker { + virDomainGetStatsFunc func; + unsigned int stats; +}; + +static struct virDomainGetStatsWorker virDomainGetStatsWorkers[] = { + { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, + { NULL, 0 } +}; + + +static int +virDomainGetStats(virConnectPtr conn, + virDomainObjPtr dom, + unsigned int stats, + virDomainStatsRecordPtr *record, + unsigned int flags) +{ + int maxparams = 0; + virDomainStatsRecordPtr tmp; + size_t i; + int ret = -1; + + if (VIR_ALLOC(tmp) < 0) + goto cleanup; + + for (i = 0; virDomainGetStatsWorkers[i].func; i++) { + if (stats & VIR_DOMAIN_STATS_ALL || + stats & virDomainGetStatsWorkers[i].stats) { + if (virDomainGetStatsWorkers[i].func(dom, tmp, &maxparams, + flags) < 0) + goto cleanup; + + if (stats & virDomainGetStatsWorkers[i].stats) + stats &= ~(stats & virDomainGetStatsWorkers[i].stats); + } + } + + if (stats & ~VIR_DOMAIN_STATS_ALL) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Stats types bits 0x%x are not supported by this daemon"), + stats); + goto cleanup; + } + + /* domain obj construction has to be last so that we don't + * need to free it on error as freeing func clears the error code */ + if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid))) + goto cleanup; + + *record = tmp; + tmp = NULL; + ret = 0; + + cleanup: + if (tmp) { + virTypedParamsFree(tmp->params, tmp->nparams); + VIR_FREE(tmp); + } + + return ret; +} + + +static int +qemuDomainListGetStats(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virQEMUDriverPtr driver = conn->privateData; + virDomainPtr *domlist = NULL; + virDomainObjPtr dom = NULL; + virDomainStatsRecordPtr *tmpstats = NULL; + int ntempdoms; + int nstats = 0; + size_t i; + int ret = -1; + + virCheckFlags(0, -1); + + if (!ndoms) { + if ((ntempdoms = virDomainObjListExport(driver->domains, + conn, + &domlist, + NULL, + 0)) < 0) + goto cleanup; + + ndoms = ntempdoms; + doms = domlist; + } + + if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) + goto cleanup; + + for (i = 0; i < ndoms; i++) { + virDomainStatsRecordPtr tmp = NULL; + + if (!(dom = qemuDomObjFromDomain(doms[i]))) { + if (domlist) + continue; + } + + if (!domlist && + virDomainListGetStatsEnsureACL(conn, dom->def) < 0) + continue; + + if (virDomainGetStats(conn, dom, stats, &tmp, flags) < 0) + goto cleanup; + + if (tmp) + tmpstats[nstats++] = tmp; + + virObjectUnlock(dom); + dom = NULL; + } + + *retStats = tmpstats; + tmpstats = NULL; + + ret = nstats; + + cleanup: + if (dom) + virObjectUnlock(dom); + + if (tmpstats) + virDomainStatsRecordListFree(tmpstats); + + if (domlist) { + for (i = 0; i < ndoms; i++) + virObjectUnref(domlist[i]); + + VIR_FREE(domlist); + } + + return ret; +} + + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -17308,6 +17482,7 @@ static virDriver qemuDriver = { .domainSetTime = qemuDomainSetTime, /* 1.2.5 */ .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */ + .domainListGetStats = qemuDomainListGetStats, /* 1.2.8 */ }; -- 2.0.2
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Implement the API function for virDomainListGetStats and virConnectGetAllDomainStats in a modular way and implement the VIR_DOMAIN_STATS_STATE group of statistics. --- src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+)
This API should also be fairly easy to implement for the test:// driver, which would make it also easier to test how well it does. How hard would it be for you to copy the ideas of this patch into a patch 6?
+static int +qemuDomainGetStatsState(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int privflags ATTRIBUTE_UNUSED)
+ return -1; + + + return 0;
Why two blank lines?
+} + + +typedef int +(*virDomainGetStatsFunc)(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags); + +struct virDomainGetStatsWorker { + virDomainGetStatsFunc func; + unsigned int stats; +}; + +static struct virDomainGetStatsWorker virDomainGetStatsWorkers[] = { + { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, + { NULL, 0 } +};
For example, the idea of virDomainGetStatsFunc as a callback may fit better under src/conf/domain* for easier sharing, while the specific implementation of the array belongs here in qemu_driver.c.
+ + +static int +virDomainGetStats(virConnectPtr conn, + virDomainObjPtr dom, + unsigned int stats, + virDomainStatsRecordPtr *record, + unsigned int flags) +{ + int maxparams = 0; + virDomainStatsRecordPtr tmp; + size_t i; + int ret = -1; + + if (VIR_ALLOC(tmp) < 0) + goto cleanup;
Is it necessary to malloc this, or can you just stack-allocate it and pass &tmp to other functions?
+ + for (i = 0; virDomainGetStatsWorkers[i].func; i++) { + if (stats & VIR_DOMAIN_STATS_ALL || + stats & virDomainGetStatsWorkers[i].stats) {
Again, per my question on 1/5, I'm thinking the logic here might be better as: if (!stats || stats & virDomainGetStatsWorkers[i].stats) coupled with logic that says if stats has any bits that did not have at least one worker, but flags says to enforce all stats bits, then fail (actually, the logic of an impossible stats bit request can be hoisted to the caller - it is a one-time check outside the loop over all domains, while this is the code being done on the inner loop once per domain).
+ + if (stats & ~VIR_DOMAIN_STATS_ALL) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Stats types bits 0x%x are not supported by this daemon"), + stats); + goto cleanup; + }
As above, this check can be hoisted outside the outer loop of the caller (since neither the outer loop nor inner loop is changing the user's requested set of stats).
+static int +qemuDomainListGetStats(virConnectPtr conn, + virDomainPtr *doms, + unsigned int ndoms, + unsigned int stats, + virDomainStatsRecordPtr **retStats, + unsigned int flags) +{ + virQEMUDriverPtr driver = conn->privateData; + virDomainPtr *domlist = NULL; + virDomainObjPtr dom = NULL; + virDomainStatsRecordPtr *tmpstats = NULL; + int ntempdoms; + int nstats = 0; + size_t i; + int ret = -1; + + virCheckFlags(0, -1);
I think you want to call virDomainListGetStatsEnsureACL up front. Ouch - I see what's going on. The ACL generator thinks that because your API prefix is virDomain, that the ACL is per-domain, when in reality, you are doing a virConnect operation. I think you need to rename the driver.h callback in patch 1/5 so that the signature for the ACL check here becomes: virConnectDomainListGetStatsEnsureACL(conn) to prove whether we can even use this API, then later...
+ + if (!ndoms) { + if ((ntempdoms = virDomainObjListExport(driver->domains, + conn, + &domlist, + NULL, + 0)) < 0)
...here, when gathering the list of domains you can iterate on, you want to pass the filter function virConnectDomainListGetStatsCheckACL, rather than a NULL filter. That way, your list omits domains that the user has no right to access, before wasting time collecting stats on those domains.
+ goto cleanup; + + ndoms = ntempdoms; + doms = domlist; + } + + if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) + goto cleanup; + + for (i = 0; i < ndoms; i++) { + virDomainStatsRecordPtr tmp = NULL; + + if (!(dom = qemuDomObjFromDomain(doms[i]))) { + if (domlist) + continue; + } + + if (!domlist && + virDomainListGetStatsEnsureACL(conn, dom->def) < 0) + continue;
Hmm, here, if this is the global API, tmpstats should already have been filtered; but is this is the dom-list API, the user may have passed in a domain that they can list (virConnectListAllDomains only filters on domain:getattr) but not read (most stat functions filter on domain:read, which requires more permissions than domain:getattr). So maybe you need filtering here, too, but only when domlist is non-NULL. Maybe we should get Dan's review on this (since he wrote ACL). I'm fine if we commit to the API before freeze, even if we don't get the implementation reviewed for a couple more days - I'd rather get it right and not release a bug that could turn into a CVE for revealing too much information to an unprivileged user that should not have been able to read stats on a domain.
+ cleanup: + if (dom) + virObjectUnlock(dom); +
Useless if. virObjectUnlock(NULL) is safe. (Hmm, why isn't syntax-check catching it?)
+ if (tmpstats) + virDomainStatsRecordListFree(tmpstats);
Another useless if. We should make this function play nicely with a NULL argument.
+ + if (domlist) {
Another useless if, since you took care to initialize ndoms = 0 and only increment it after you have a domlist.
+ for (i = 0; i < ndoms; i++) + virObjectUnref(domlist[i]); + + VIR_FREE(domlist); + } + + return ret; +} + + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -17308,6 +17482,7 @@ static virDriver qemuDriver = { .domainSetTime = qemuDomainSetTime, /* 1.2.5 */ .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */ + .domainListGetStats = qemuDomainListGetStats, /* 1.2.8 */ };
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/14 21:02, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Implement the API function for virDomainListGetStats and virConnectGetAllDomainStats in a modular way and implement the VIR_DOMAIN_STATS_STATE group of statistics. --- src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+)
+ cleanup: + if (dom) + virObjectUnlock(dom); +
Useless if. virObjectUnlock(NULL) is safe. (Hmm, why isn't syntax-check catching it?)
Actually not useless. virObjectUnlock should not be called with NULL. This doesn't crash but spews warnings: /** * virObjectUnlock: * @anyobj: any instance of virObjectLockablePtr * * Release a lock on @anyobj. The lock must have been * acquired by virObjectLock. */ void virObjectUnlock(void *anyobj) { virObjectLockablePtr obj = anyobj; if (!virObjectIsClass(obj, virObjectLockableClass)) { VIR_WARN("Object %p (%s) is not a virObjectLockable instance", obj, obj ? obj->parent.klass->name : "(unknown)"); return; } virMutexUnlock(&obj->lock); } /** * virObjectIsClass: * @anyobj: any instance of virObjectPtr * @klass: the class to check * * Checks whether @anyobj is an instance of * @klass * * Returns true if @anyobj is an instance of @klass */ bool virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; if (!obj) return false; return virClassIsDerivedFrom(obj->klass, klass); } Peter
On 08/26/2014 01:06 PM, Peter Krempa wrote:
On 08/26/14 21:02, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Implement the API function for virDomainListGetStats and virConnectGetAllDomainStats in a modular way and implement the VIR_DOMAIN_STATS_STATE group of statistics. --- src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+)
+ cleanup: + if (dom) + virObjectUnlock(dom); +
Useless if. virObjectUnlock(NULL) is safe. (Hmm, why isn't syntax-check catching it?)
Actually not useless. virObjectUnlock should not be called with NULL. This doesn't crash but spews warnings:
Oh, I was confusing it with virObjectUnref(NULL), which IS safe. Carry on, nothing to see here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Split out guts of the function to reuse it to get domain objects from string. --- tools/virsh-domain.c | 80 +++++++++++++++++++++++++++++++++------------------- tools/virsh-domain.h | 4 +++ 2 files changed, 55 insertions(+), 29 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2562326..b28de5a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -60,57 +60,79 @@ # define SA_SIGINFO 0 #endif -virDomainPtr -vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, - const char **name, unsigned int flags) + +static virDomainPtr +vshLookupDomainInternal(vshControl *ctl, + const char *cmdname, + const char *name, + unsigned int flags) { virDomainPtr dom = NULL; - const char *n = NULL; int id; - const char *optname = "domain"; virCheckFlags(VSH_BYID | VSH_BYUUID | VSH_BYNAME, NULL); - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; - - if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) - return NULL; - - vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", - cmd->def->name, optname, n); - - if (name) - *name = n; - /* try it by ID */ if (flags & VSH_BYID) { - if (virStrToLong_i(n, NULL, 10, &id) == 0 && id >= 0) { - vshDebug(ctl, VSH_ERR_DEBUG, - "%s: <%s> seems like domain ID\n", - cmd->def->name, optname); + if (virStrToLong_i(name, NULL, 10, &id) == 0 && id >= 0) { + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <domain> looks like ID\n", + cmdname); dom = virDomainLookupByID(ctl->conn, id); } } + /* try it by UUID */ if (!dom && (flags & VSH_BYUUID) && - strlen(n) == VIR_UUID_STRING_BUFLEN-1) { - vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as domain UUID\n", - cmd->def->name, optname); - dom = virDomainLookupByUUIDString(ctl->conn, n); + strlen(name) == VIR_UUID_STRING_BUFLEN-1) { + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <domain> trying as domain UUID\n", + cmdname); + dom = virDomainLookupByUUIDString(ctl->conn, name); } + /* try it by NAME */ if (!dom && (flags & VSH_BYNAME)) { - vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as domain NAME\n", - cmd->def->name, optname); - dom = virDomainLookupByName(ctl->conn, n); + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <domain> trying as domain NAME\n", + cmdname); + dom = virDomainLookupByName(ctl->conn, name); } if (!dom) - vshError(ctl, _("failed to get domain '%s'"), n); + vshError(ctl, _("failed to get domain '%s'"), name); return dom; } + +virDomainPtr +vshLookupDomainBy(vshControl *ctl, + const char *name, + unsigned int flags) +{ + return vshLookupDomainInternal(ctl, "unknown", name, flags); +} + + +virDomainPtr +vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, + const char **name, unsigned int flags) +{ + const char *n = NULL; + const char *optname = "domain"; + + if (!vshCmdHasOption(ctl, cmd, optname)) + return NULL; + + if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) + return NULL; + + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", + cmd->def->name, optname, n); + + if (name) + *name = n; + + return vshLookupDomainInternal(ctl, cmd->def->name, n, flags); +} + VIR_ENUM_DECL(vshDomainVcpuState) VIR_ENUM_IMPL(vshDomainVcpuState, VIR_VCPU_LAST, diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index f03a0bb..f46538f 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -28,6 +28,10 @@ # include "virsh.h" +virDomainPtr vshLookupDomainBy(vshControl *ctl, + const char *name, + unsigned int flags); + virDomainPtr vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, const char **name, unsigned int flags); -- 2.0.2
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Split out guts of the function to reuse it to get domain objects from string. --- tools/virsh-domain.c | 80 +++++++++++++++++++++++++++++++++------------------- tools/virsh-domain.h | 4 +++ 2 files changed, 55 insertions(+), 29 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/14 22:46, Eric Blake wrote:
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Split out guts of the function to reuse it to get domain objects from string. --- tools/virsh-domain.c | 80 +++++++++++++++++++++++++++++++++------------------- tools/virsh-domain.h | 4 +++ 2 files changed, 55 insertions(+), 29 deletions(-)
ACK
Pushed; Thanks. Peter
Add "domstats" command that excercises both of the new APIs depending if you specify a domain list or not. The output is printed as a key=value list of the returned parameters. Man page section will be added in a separate patch. --- tools/virsh-domain-monitor.c | 140 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8bd58ad..8eb3a21 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1954,6 +1954,140 @@ cmdList(vshControl *ctl, const vshCmd *cmd) } #undef FILTER +/* + * "domstats" command + */ +static const vshCmdInfo info_domstats[] = { + {.name = "help", + .data = N_("get statistics about one or multiple domains") + }, + {.name = "desc", + .data = N_("Gets statistics about one or more (or all) domains") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_domstats[] = { + {.name = "stats", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("bit map of stats to retrieve"), + }, + {.name = "allstats", + .type = VSH_OT_BOOL, + .help = N_("report all stats supported by the hypervisor"), + }, + {.name = "state", + .type = VSH_OT_BOOL, + .help = N_("report domain state"), + }, + {.name = "domains", + .type = VSH_OT_ARGV, + .flags = VSH_OFLAG_NONE, + .help = N_("list of domains to get stats for"), + }, + {.name = NULL} +}; + +static bool +vshDomainStatsPrint(vshControl *ctl ATTRIBUTE_UNUSED, + virDomainStatsRecordPtr record) +{ + char *param; + size_t i; + + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom)); + + for (i = 0; i < record->nparams; i++) { + if (!(param = vshGetTypedParamValue(ctl, record->params + i))) + return false; + + vshPrint(ctl, " %s=%s\n", record->params[i].field, param); + + VIR_FREE(param); + } + + vshPrint(ctl, "\n"); + return true; +} + +static bool +cmdDomstats(vshControl *ctl, const vshCmd *cmd) +{ + unsigned int stats = 0; + virDomainPtr *domlist = NULL; + virDomainPtr *nextdom; + virDomainPtr dom; + size_t ndoms = 0; + virDomainStatsRecordPtr *records = NULL; + virDomainStatsRecordPtr *next; + int flags = 0; + const vshCmdOpt *opt = NULL; + bool ret = false; + + if (vshCommandOptUInt(cmd, "stats", &stats) < 0) { + vshError(ctl, "%s", _("Unable to parse stats bitmap")); + return false; + } + + if (vshCommandOptBool(cmd, "allstats")) + stats |= VIR_DOMAIN_STATS_ALL; + + if (vshCommandOptBool(cmd, "state")) + stats |= VIR_DOMAIN_STATS_STATE; + + if (stats == 0) + stats = VIR_DOMAIN_STATS_ALL; + + if (vshCommandOptBool(cmd, "domains")) { + if (VIR_ALLOC_N(domlist, 1) < 0) + goto cleanup; + ndoms = 1; + + while ((opt = vshCommandOptArgv(cmd, opt))) { + if (!(dom = vshLookupDomainBy(ctl, opt->data, + VSH_BYID | VSH_BYUUID | VSH_BYNAME))) + goto cleanup; + + if (VIR_INSERT_ELEMENT(domlist, 0, ndoms, dom) < 0) + goto cleanup; + } + + if (virDomainListGetStats(domlist, + stats, + &records, + flags) < 0) + goto cleanup; + } else { + if ((virConnectGetAllDomainStats(ctl->conn, + stats, + &records, + flags)) < 0) + goto cleanup; + } + + if (records) { + for (next = records; *next; next++) { + if (!vshDomainStatsPrint(ctl, *next)) + goto cleanup; + } + } + + ret = true; + cleanup: + if (records) + virDomainStatsRecordListFree(records); + + if (domlist) { + for (nextdom = domlist; *nextdom; nextdom++) + virDomainFree(*nextdom); + + VIR_FREE(domlist); + } + + return ret; +} + const vshCmdDef domMonitoringCmds[] = { {.name = "domblkerror", .handler = cmdDomBlkError, @@ -2021,6 +2155,12 @@ const vshCmdDef domMonitoringCmds[] = { .info = info_domstate, .flags = 0 }, + {.name = "domstats", + .handler = cmdDomstats, + .opts = opts_domstats, + .info = info_domstats, + .flags = 0 + }, {.name = "domtime", .handler = cmdDomTime, .opts = opts_domtime, -- 2.0.2
On 08/26/2014 08:14 AM, Peter Krempa wrote:
Add "domstats" command that excercises both of the new APIs depending if you specify a domain list or not. The output is printed as a key=value list of the returned parameters.
Man page section will be added in a separate patch.
Trying to stretch those release deadlines as long as possible, aren't you ;)
--- tools/virsh-domain-monitor.c | 140 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8bd58ad..8eb3a21 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1954,6 +1954,140 @@ cmdList(vshControl *ctl, const vshCmd *cmd) } #undef FILTER
+/* + * "domstats" command + */ +static const vshCmdInfo info_domstats[] = { + {.name = "help", + .data = N_("get statistics about one or multiple domains") + }, + {.name = "desc", + .data = N_("Gets statistics about one or more (or all) domains") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_domstats[] = { + {.name = "stats", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("bit map of stats to retrieve"), + },
Ewww - you're seriously going to make the user pass in a raw integer? I'd much rather you provide a series of VSH_OT_BOOL options, each of which enables a bit in the stats bitmask: the user should be able to say --state, rather than guessing that --stats=1 does what they want.
+ {.name = "allstats", + .type = VSH_OT_BOOL, + .help = N_("report all stats supported by the hypervisor"),
Is that necessary? Shouldn't that just be the default you get when stats is 0? And if you do boolean flags for each possible category, omitting all of the booleans leaves you with stats==0 to begin with. I'd drop this one.
+ }, + {.name = "state", + .type = VSH_OT_BOOL, + .help = N_("report domain state"), + },
Wait - so you have --stats=INT _and_ --state as a bool? That's too confusing. I'd drop --stats.
+ {.name = "domains", + .type = VSH_OT_ARGV, + .flags = VSH_OFLAG_NONE, + .help = N_("list of domains to get stats for"), + },
Yay - this one works.
+static bool +vshDomainStatsPrint(vshControl *ctl ATTRIBUTE_UNUSED, + virDomainStatsRecordPtr record) +{ + char *param; + size_t i; + + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom)); + + for (i = 0; i < record->nparams; i++) { + if (!(param = vshGetTypedParamValue(ctl, record->params + i))) + return false; + + vshPrint(ctl, " %s=%s\n", record->params[i].field, param);
Hmm - should we attempt to pretty-print things, such as state.status=running instead of state.status=1? But to do that, we'd have to special-case which record->paramsi].field names are worth pretty-printing. It can be done as a followup patch. In fact, I'm wondering if we should have a --raw (and/or a --pretty) that forces the output to be raw int vs. pretty name at the user's choice (probably pretty by default, raw by request), if it makes machine-parsing of the output any easier. But now that I've typed that, I'm not sure if it is easier - as long as we don't have any potential for ambiguous output, a machine can parse 'state.status=running' just about as easily as 'state.status=1'.
+ if (vshCommandOptUInt(cmd, "stats", &stats) < 0) { + vshError(ctl, "%s", _("Unable to parse stats bitmap")); + return false; + }
I don't think exposing this to the user as a raw int provides any benefits. Drop it.
+ + if (vshCommandOptBool(cmd, "allstats")) + stats |= VIR_DOMAIN_STATS_ALL;
Probably don't need this one either, once stats==0 implies all stats.
+ + while ((opt = vshCommandOptArgv(cmd, opt))) { + if (!(dom = vshLookupDomainBy(ctl, opt->data, + VSH_BYID | VSH_BYUUID | VSH_BYNAME))) + goto cleanup; + + if (VIR_INSERT_ELEMENT(domlist, 0, ndoms, dom) < 0)
Why are you inserting at the front? If I call 'domstats a b', I'd expect the stats for 'a' to be output first, not last.
+ cleanup: + if (records) + virDomainStatsRecordListFree(records);
Useless if, if you take my suggestion in patch 1. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa