9:14 a.m.
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
9:14 a.m.
New subject: [libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats
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
11:49 a.m.
New subject: [libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats
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
12:28 p.m.
New subject: [libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats
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
1:27 p.m.
New subject: [libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats
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
2:11 p.m.
New subject: [libvirt] [PATCHv3] lib: Introduce API for retrieving bulk domain stats
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
2:41 p.m.
New subject: [libvirt] [PATCHv3] lib: Introduce API for retrieving bulk domain stats
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
3:55 p.m.
New subject: [libvirt] [PATCHv3] lib: Introduce API for retrieving bulk domain stats
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
9:14 a.m.
New subject: [libvirt] [PATCHv2 2/5] remote: Implement bulk domain stats APIs in the remote driver
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
1:22 p.m.
New subject: [libvirt] [PATCHv2 2/5] remote: Implement bulk domain stats APIs in the remote driver
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
1:49 p.m.
New subject: [libvirt] [PATCHv2 2/5] remote: Implement bulk domain stats APIs in the remote driver
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
9:14 a.m.
New subject: [libvirt] [PATCHv2 3/5] qemu: Implement bulk stats API and one of the stats groups to return
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
2:02 p.m.
New subject: [libvirt] [PATCHv2 3/5] qemu: Implement bulk stats API and one of the stats groups to return
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
2:06 p.m.
New subject: [libvirt] [PATCHv2 3/5] qemu: Implement bulk stats API and one of the stats groups to return
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
2:14 p.m.
New subject: [libvirt] [PATCHv2 3/5] qemu: Implement bulk stats API and one of the stats groups to return
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
9:14 a.m.
New subject: [libvirt] [PATCHv2 4/5] virsh: domain: Split out code to lookup domain from string
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
3:46 p.m.
New subject: [libvirt] [PATCHv2 4/5] virsh: domain: Split out code to lookup domain from string
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
3:56 p.m.
New subject: [libvirt] [PATCHv2 4/5] virsh: domain: Split out code to lookup domain from string
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
9:14 a.m.
New subject: [libvirt] [PATCHv2 5/5] virsh: Implement command to excercise the bulk stats APIs
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
2:58 p.m.
New subject: [libvirt] [PATCHv2 5/5] virsh: Implement command to excercise the bulk stats APIs
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
3771
days inactive
3771
days old
19 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa