[libvirt] [PATCHv3 0/5] Implement bulk stats API
New iteration of the series with a few improvements Peter Krempa (5): conf: Add helper to free domain list lib: Add few flags for the bulk stats APIs remote: Implement bulk domain stats APIs in the remote driver qemu: Implement bulk stats API and one of the stats groups to return virsh: Implement command to excercise the bulk stats APIs daemon/remote.c | 86 +++++++++++++++++++ include/libvirt/libvirt.h.in | 15 ++++ src/conf/domain_conf.c | 31 +++++-- src/conf/domain_conf.h | 2 + src/libvirt.c | 29 ++++++- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 84 +++++++++++++++++++ src/remote/remote_protocol.x | 25 +++++- src/remote_protocol-structs | 22 +++++ tools/virsh-domain-monitor.c | 191 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 34 ++++++++ 12 files changed, 685 insertions(+), 10 deletions(-) -- 2.0.2
Add helper to free a list of virDomainPtrs without raising or clearing errors. Use it in one place and prepare it for reuse. --- src/conf/domain_conf.c | 31 +++++++++++++++++++++++-------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b295ab..e9ed3fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20016,6 +20016,28 @@ virDomainListPopulate(void *payload, } #undef MATCH + +/** + * virDomainListFree: + * @list: list of domains to free + * + * Frees the list of domains without messing with currently set libvirt errors. + */ +void +virDomainListFree(virDomainPtr *list) +{ + virDomainPtr *next; + + if (!list) + return; + + for (next = list; *next; next++) + virObjectUnref(*next); + + VIR_FREE(list); +} + + int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, @@ -20024,7 +20046,6 @@ virDomainObjListExport(virDomainObjListPtr doms, unsigned int flags) { int ret = -1; - size_t i; struct virDomainListData data = { conn, NULL, @@ -20052,13 +20073,7 @@ virDomainObjListExport(virDomainObjListPtr doms, ret = data.ndomains; cleanup: - if (data.domains) { - int count = virHashSize(doms->objs); - for (i = 0; i < count; i++) - virObjectUnref(data.domains[i]); - } - - VIR_FREE(data.domains); + virDomainListFree(data.domains); virObjectUnlock(doms); return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index aead903..a05254a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2707,6 +2707,8 @@ int virDomainObjListExport(virDomainObjListPtr doms, virDomainObjListFilter filter, unsigned int flags); +void virDomainListFree(virDomainPtr *list); + int virDomainDefMaybeAddController(virDomainDefPtr def, int type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6b9ee21..71fc063 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -311,6 +311,7 @@ virDomainLifecycleCrashTypeFromString; virDomainLifecycleCrashTypeToString; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; +virDomainListFree; virDomainLiveConfigHelperMethod; virDomainLockFailureTypeFromString; virDomainLockFailureTypeToString; -- 2.0.2
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Add helper to free a list of virDomainPtrs without raising or clearing errors. Use it in one place and prepare it for reuse. --- src/conf/domain_conf.c | 31 +++++++++++++++++++++++-------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+), 8 deletions(-)
ACK with one minor suggestion.
+/** + * virDomainListFree: + * @list: list of domains to free + * + * Frees the list of domains without messing with currently set libvirt errors.
Maybe mention that the list must be NULL-terminated.
@@ -20052,13 +20073,7 @@ virDomainObjListExport(virDomainObjListPtr doms, ret = data.ndomains;
cleanup: - if (data.domains) { - int count = virHashSize(doms->objs); - for (i = 0; i < count; i++) - virObjectUnref(data.domains[i]); - } - - VIR_FREE(data.domains); + virDomainListFree(data.domains);
I had to check, but yes, this conversion is correct (data.domains was allocated earlier as virHashSize(doms->objs)+1, so it is NULL-terminated without having to grab the hash size a second time). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add domain list filtering functions and a flag to enforce checking whether the remote daemon supports the requested stats groups. --- include/libvirt/libvirt.h.in | 15 +++++++++++++++ src/libvirt.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e79c9ad..9358314 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,21 @@ typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ } virDomainStatsTypes; +typedef enum { + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = VIR_CONNECT_LIST_DOMAINS_ACTIVE, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = VIR_CONNECT_LIST_DOMAINS_INACTIVE, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = VIR_CONNECT_LIST_DOMAINS_PERSISTENT, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = VIR_CONNECT_LIST_DOMAINS_TRANSIENT, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = VIR_CONNECT_LIST_DOMAINS_RUNNING, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = VIR_CONNECT_LIST_DOMAINS_PAUSED, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = VIR_CONNECT_LIST_DOMAINS_SHUTOFF, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested stats */ +} virConnectGetAllDomainStatsFlags; + int virConnectGetAllDomainStats(virConnectPtr conn, unsigned int stats, virDomainStatsRecordPtr **retStats, diff --git a/src/libvirt.c b/src/libvirt.c index 17ec679..cae93f9 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21549,6 +21549,26 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * + * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes + * the function return error in case some of the stat types in @stats were + * not recognized by the daemon. + * + * Similarly to virConnectListAllDomains @flags can contain various flags to + * filter the list of domains to provide stats for. + * + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE selects online domains while + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE selects offline ones. + * + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT and + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT allow to filter the list + * according to their persistence. + * + * To filter the list of VMs by domain state @flags can contain + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING, + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED, + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF and/or + * VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states. + * * 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. @@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn, * @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 + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags; * * Query statistics for domains provided by @doms. Note that all domains in * @doms must share the same connection. @@ -21615,6 +21635,13 @@ virConnectGetAllDomainStats(virConnectPtr conn, * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * + * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes + * the function return error in case some of the stat types in @stats were + * not recognized by the daemon. + * + * Note that specifying any of the domain list filtering flags in @flags + * doesn't have effect on this function. + * * 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. -- 2.0.2
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Add domain list filtering functions and a flag to enforce checking whether the remote daemon supports the requested stats groups. --- include/libvirt/libvirt.h.in | 15 +++++++++++++++ src/libvirt.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e79c9ad..9358314 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,21 @@ typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ } virDomainStatsTypes;
+typedef enum { + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = VIR_CONNECT_LIST_DOMAINS_ACTIVE, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = VIR_CONNECT_LIST_DOMAINS_INACTIVE, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = VIR_CONNECT_LIST_DOMAINS_PERSISTENT, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = VIR_CONNECT_LIST_DOMAINS_TRANSIENT, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = VIR_CONNECT_LIST_DOMAINS_RUNNING, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = VIR_CONNECT_LIST_DOMAINS_PAUSED, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = VIR_CONNECT_LIST_DOMAINS_SHUTOFF, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER,
Is this going to confuse the python bindings? (I know in other places where we have set up one enum to match another that it didn't seem to work). But fixing that would be a separate patch; it may be that docs/apibuild.py needs to learn how to do symbolic name dereferencing before generating the xml that feeds the python bindings (see also commit 9b291bb).
+ * + * Similarly to virConnectListAllDomains @flags can contain various flags to
s/ListAllDomains/ListAllDomains,/
@@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn, * @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 + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags;
Why the trailing semicolon?
+ * + * Note that specifying any of the domain list filtering flags in @flags + * doesn't have effect on this function.
Fair enough, I suppose. But I'd rather we error out if filtering flags were requested, instead of silently ignoring them, so that if we later change our mind, we can do in-place filtering. I'll probably have more to say on this on a later patch. Looks reasonable, so ACK with the grammar fixes -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/27/14 22:50, Eric Blake wrote:
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Add domain list filtering functions and a flag to enforce checking whether the remote daemon supports the requested stats groups. --- include/libvirt/libvirt.h.in | 15 +++++++++++++++ src/libvirt.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e79c9ad..9358314 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,21 @@ typedef enum { VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */ } virDomainStatsTypes;
+typedef enum { + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = VIR_CONNECT_LIST_DOMAINS_ACTIVE, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = VIR_CONNECT_LIST_DOMAINS_INACTIVE, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = VIR_CONNECT_LIST_DOMAINS_PERSISTENT, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = VIR_CONNECT_LIST_DOMAINS_TRANSIENT, + + VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = VIR_CONNECT_LIST_DOMAINS_RUNNING, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = VIR_CONNECT_LIST_DOMAINS_PAUSED, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = VIR_CONNECT_LIST_DOMAINS_SHUTOFF, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER,
Is this going to confuse the python bindings? (I know in other places where we have set up one enum to match another that it didn't seem to work). But fixing that would be a separate patch; it may be that docs/apibuild.py needs to learn how to do symbolic name dereferencing before generating the xml that feeds the python bindings (see also commit 9b291bb).
+ * + * Similarly to virConnectListAllDomains @flags can contain various flags to
s/ListAllDomains/ListAllDomains,/
@@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn, * @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 + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags;
Why the trailing semicolon?
+ * + * Note that specifying any of the domain list filtering flags in @flags + * doesn't have effect on this function.
Fair enough, I suppose. But I'd rather we error out if filtering flags were requested, instead of silently ignoring them, so that if we later change our mind, we can do in-place filtering. I'll probably have more to say on this on a later patch.
I changed this sentence to: * Note that any of the domain list filtering flags in @flags will be rejected * by this function. *
Looks reasonable, so ACK with the grammar fixes
And all the other tweaks you've suggested. Peter
Implement the remote driver support for shuffling the domain stats around. --- daemon/remote.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 84 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 25 ++++++++++++- src/remote_protocol-structs | 22 ++++++++++++ 4 files changed, 216 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 9251576..c752479 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6383,6 +6383,92 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, } +static int +remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_all_domain_stats_args *args, + remote_connect_get_all_domain_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_CONNECT_GET_ALL_DOMAIN_STATS_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 (nrecords) { + ret->retStats.retStats_len = nrecords; + + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) + goto cleanup; + + for (i = 0; i < nrecords; i++) { + remote_domain_stats_record *dst = ret->retStats.retStats_val + i; + + make_nonnull_domain(&dst->dom, retStats[i]->dom); + + if (remoteSerializeTypedParameters(retStats[i]->params, + retStats[i]->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; + } + + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + + virDomainStatsRecordListFree(retStats); + virDomainListFree(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 e949223..fda27f7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7717,6 +7717,89 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net, } +static int +remoteConnectGetAllDomainStats(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_connect_get_all_domain_stats_args args; + remote_connect_get_all_domain_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_CONNECT_GET_ALL_DOMAIN_STATS, + (xdrproc_t)xdr_remote_connect_get_all_domain_stats_args, (char *)&args, + (xdrproc_t)xdr_remote_connect_get_all_domain_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 (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_CONNECT_GET_ALL_DOMAIN_STATS_MAX, + &elem->params, + &elem->nparams)) + goto cleanup; + + tmpret[i] = elem; + } + + *retStats = tmpret; + tmpret = NULL; + rv = ret.retStats.retStats_len; + + cleanup: + virDomainStatsRecordListFree(tmpret); + xdr_free((xdrproc_t)xdr_remote_connect_get_all_domain_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, @@ -8056,6 +8139,7 @@ static virDriver remote_driver = { .domainSetTime = remoteDomainSetTime, /* 1.2.5 */ .nodeGetFreePages = remoteNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */ + .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 6dc2d29..8fc552f 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_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -3063,6 +3066,20 @@ struct remote_network_get_dhcp_leases_ret { unsigned int ret; }; +struct remote_domain_stats_record { + remote_nonnull_domain dom; + remote_typed_param params<REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX>; +}; + +struct remote_connect_get_all_domain_stats_args { + remote_nonnull_domain doms<REMOTE_DOMAIN_LIST_MAX>; + unsigned int stats; + unsigned int flags; +}; + +struct remote_connect_get_all_domain_stats_ret { + remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; +}; /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -5432,6 +5449,12 @@ enum remote_procedure { * @generate: none * @acl: domain:open_graphics */ - REMOTE_PROC_DOMAIN_OPEN_GRAPHICS_FD = 343 + REMOTE_PROC_DOMAIN_OPEN_GRAPHICS_FD = 343, + /** + * @generate: none + * @acl: connect:search_domains + * @aclfilter: domain:read + */ + REMOTE_PROC_CONNECT_GET_ALL_DOMAIN_STATS = 344 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 38bf0b8..899f1cc 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2524,6 +2524,27 @@ 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_connect_get_all_domain_stats_args { + struct { + u_int doms_len; + remote_nonnull_domain * doms_val; + } doms; + u_int stats; + u_int flags; +}; +struct remote_connect_get_all_domain_stats_ret { + struct { + u_int retStats_len; + remote_domain_stats_record * retStats_val; + } retStats; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2868,4 +2889,5 @@ enum remote_procedure { REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 341, REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342, REMOTE_PROC_DOMAIN_OPEN_GRAPHICS_FD = 343, + REMOTE_PROC_CONNECT_GET_ALL_DOMAIN_STATS = 344, }; -- 2.0.2
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Implement the remote driver support for shuffling the domain stats around. ---
+static int +remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_all_domain_stats_args *args, + remote_connect_get_all_domain_stats_ret *ret) +{ + int rv = -1;
+ if (nrecords) { + ret->retStats.retStats_len = nrecords; + + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) + goto cleanup; +
I'd switch these for safety (I don't know if the rpc cleanup code will try to dereference a NULL retStats.retStats_val if retStats_len is non-zero; while setting the length after the allocation should always be safe).
+ for (i = 0; i < nrecords; i++) { + remote_domain_stats_record *dst = ret->retStats.retStats_val + i; + + make_nonnull_domain(&dst->dom, retStats[i]->dom);
[Not your fault, and certainly not for this patch, but we REALLY ought to fix make_nonnull_domain and friends to properly return errors on OOM, instead of silently breaking things]
+++ b/src/remote/remote_driver.c @@ -7717,6 +7717,89 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net, }
+static int +remoteConnectGetAllDomainStats(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_connect_get_all_domain_stats_args args; + remote_connect_get_all_domain_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]);
[same complaint here about silent OOM bugs] ACK with the statement reorder. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/27/14 23:02, Eric Blake wrote:
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Implement the remote driver support for shuffling the domain stats around. ---
+static int +remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_all_domain_stats_args *args, + remote_connect_get_all_domain_stats_ret *ret) +{ + int rv = -1;
+ if (nrecords) { + ret->retStats.retStats_len = nrecords; + + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) + goto cleanup; +
I'd switch these for safety (I don't know if the rpc cleanup code will try to dereference a NULL retStats.retStats_val if retStats_len is non-zero; while setting the length after the allocation should always be safe).
+ for (i = 0; i < nrecords; i++) { + remote_domain_stats_record *dst = ret->retStats.retStats_val + i; + + make_nonnull_domain(&dst->dom, retStats[i]->dom);
[Not your fault, and certainly not for this patch, but we REALLY ought to fix make_nonnull_domain and friends to properly return errors on OOM, instead of silently breaking things]
+++ b/src/remote/remote_driver.c @@ -7717,6 +7717,89 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net, }
+static int +remoteConnectGetAllDomainStats(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_connect_get_all_domain_stats_args args; + remote_connect_get_all_domain_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]);
[same complaint here about silent OOM bugs]
ACK with the statement reorder.
I've fixed and pushed patches 1-3 and will follow up with v4 of the rest. Peter
Implement the API function for virDomainListGetStats and virConnectGetAllDomainStats in a modular way and implement the VIR_DOMAIN_STATS_STATE group of statistics. Although it may look like the function looks universal I'd rather not expose it to other drivers as the comming stats groups are likely to do qemu specific stuff to obtain the stats. --- 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 2db507f..b243dc3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17190,6 +17190,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 +(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, + virDomainStatsRecordPtr record, + int *maxparams, + unsigned int flags); + +struct qemuDomainGetStatsWorker { + qemuDomainGetStatsFunc func; + unsigned int stats; +}; + +static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { + { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, + { NULL, 0 } +}; + + +static int +qemuDomainGetStats(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; qemuDomainGetStatsWorkers[i].func; i++) { + if (stats == 0 || + stats & qemuDomainGetStatsWorkers[i].stats) { + if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams, + flags) < 0) + goto cleanup; + + stats &= ~(stats & qemuDomainGetStatsWorkers[i].stats); + } + } + + if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS && + stats != 0) { + 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 +qemuConnectGetAllDomainStats(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(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE | + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); + + if (virConnectGetAllDomainStatsEnsureACL(conn) < 0) + return -1; + + if (!ndoms) { + unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE); + + if ((ntempdoms = virDomainObjListExport(driver->domains, + conn, + &domlist, + virConnectGetAllDomainStatsCheckACL, + lflags)) < 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]))) + continue; + + if (!domlist && + !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) + continue; + + if (qemuDomainGetStats(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); + + virDomainStatsRecordListFree(tmpstats); + virDomainListFree(domlist); + + return ret; +} + + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -17387,6 +17561,7 @@ static virDriver qemuDriver = { .domainSetTime = qemuDomainSetTime, /* 1.2.5 */ .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */ + .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */ }; -- 2.0.2
On 08/27/2014 12:25 PM, 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.
Although it may look like the function looks universal I'd rather not expose it to other drivers as the comming stats groups are likely to do
s/comming/coming/
qemu specific stuff to obtain the stats.
And of course, we can always change our mind and refactor things later, if it turns to be helpful.
--- src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+)
+ +static int +qemuDomainGetStats(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; qemuDomainGetStatsWorkers[i].func; i++) { + if (stats == 0 || + stats & qemuDomainGetStatsWorkers[i].stats) { + if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams, + flags) < 0) + goto cleanup; + + stats &= ~(stats & qemuDomainGetStatsWorkers[i].stats);
Ouch. That doesn't quite work. Consider if I have STATE=1, CPU=2, and stat workers registered for both. If the user passes in stats=STATE, then on the first iteration, I see 'stats & statsworkers[0].stats' is true, so I append state stats [*], then clear out the STATE bit. On the second iteration, I now see 'stats == 0' is true, so I append cpu stats, even though the user didn't ask for them. What I'd do is a pre-pass: if (stats == 0) { for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) stats |= qemuDomainGetStatsWorkers[i].stats; } then ditch the 'stats == 0' branch in the main loop. That way, you are always doing subtractive work on 'stats', without picking up the tail of the workers. Hmm, that said, subtractive work is STILL slightly risky - suppose that I reach a point where it is easier to code two separate stat workers that both provide STATE information (for whatever reason that combining them into a single callback was too awkward - perhaps because in adding other drivers, we find that some stats are easy to provide with a common callback from src/conf, while other stats are still hypervisor-specific but without duplicating the common code). If I have statsWorkers[0] and [1] both tied to STATE, then worker 1 will not be called because worker 0 removed STATE from stats. On the other hand, it's still fairly trivial to write the hypervisor-specific worker to call the common core worker, and have only a single registration for STATE, so I'm probably over-thinking this scenario.
+ } + } + + if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS && + stats != 0) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Stats types bits 0x%x are not supported by this daemon"), + stats); + goto cleanup; + }
Even better, this 'if' can fail at most once (on the first domain being listed); if the first domain succeeded, you have wasted a conditional on every subsequent domain. You could still hoist the stats pre-pass entirely to the caller, and not even worry about manipulating 'stats' in this call.
+static int +qemuConnectGetAllDomainStats(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(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE | + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
As promised in the review of 2/5, I think you could do: /* Allow filter flags only when called via virConnect variant */ if (ndoms) virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1) else virCheckFlags(all the bits, -1) so that you forcefully reject callers from trying to do filtering when using the domlist variant of the public API. That way, if we later change our mind, and decide to let filtering work, users are guaranteed sane behavior (change from a hard error to working is better than change from silently ignored to working). I think this one needs a v4, just to make sure we get the modularity right when there is more than one stats bit to support. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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. --- tools/virsh-domain-monitor.c | 191 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 34 ++++++++ 2 files changed, 225 insertions(+) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8bd58ad..b152256 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1954,6 +1954,191 @@ 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 = "state", + .type = VSH_OT_BOOL, + .help = N_("report domain state"), + }, + {.name = "list-active", + .type = VSH_OT_BOOL, + .help = N_("list only active domains"), + }, + {.name = "list-inactive", + .type = VSH_OT_BOOL, + .help = N_("list only inactive domains"), + }, + {.name = "list-persistent", + .type = VSH_OT_BOOL, + .help = N_("list only persistent domains"), + }, + {.name = "list-transient", + .type = VSH_OT_BOOL, + .help = N_("list only transient domains"), + }, + {.name = "list-running", + .type = VSH_OT_BOOL, + .help = N_("list only running domains"), + }, + {.name = "list-paused", + .type = VSH_OT_BOOL, + .help = N_("list only paused domains"), + }, + {.name = "list-shutoff", + .type = VSH_OT_BOOL, + .help = N_("list only shutoff domains"), + }, + {.name = "list-other", + .type = VSH_OT_BOOL, + .help = N_("list only domains in other states"), + }, + {.name = "raw", + .type = VSH_OT_BOOL, + .help = N_("do not pretty-print the fields"), + }, + {.name = "enforce", + .type = VSH_OT_BOOL, + .help = N_("enforce requested stats parameters"), + }, + {.name = "domains", + .type = VSH_OT_ARGV, + .flags = VSH_OFLAG_NONE, + .help = N_("list of domains to get stats for"), + }, + {.name = NULL} +}; + + +static bool +vshDomainStatsPrintRecord(vshControl *ctl ATTRIBUTE_UNUSED, + virDomainStatsRecordPtr record, + bool raw ATTRIBUTE_UNUSED) +{ + char *param; + size_t i; + + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom)); + + /* XXX: Implement pretty-printing */ + + 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 dom; + size_t ndoms = 0; + virDomainStatsRecordPtr *records = NULL; + virDomainStatsRecordPtr *next; + bool raw = vshCommandOptBool(cmd, "raw"); + int flags = 0; + const vshCmdOpt *opt = NULL; + bool ret = false; + + VSH_EXCLUSIVE_OPTIONS("domains", "list-active"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-inactive"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-persistent"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-transient"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-running"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-paused"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-shutoff"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-other"); + + if (vshCommandOptBool(cmd, "state")) + stats |= VIR_DOMAIN_STATS_STATE; + + if (vshCommandOptBool(cmd, "list-active")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE; + + if (vshCommandOptBool(cmd, "list-inactive")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE; + + if (vshCommandOptBool(cmd, "list-persistent")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT; + + if (vshCommandOptBool(cmd, "list-transient")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT; + + if (vshCommandOptBool(cmd, "list-running")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING; + + if (vshCommandOptBool(cmd, "list-paused")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED; + + if (vshCommandOptBool(cmd, "list-shutoff")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF; + + if (vshCommandOptBool(cmd, "list-other")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER; + + if (vshCommandOptBool(cmd, "enforce")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS; + + 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, ndoms - 1, 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; + } + + for (next = records; *next; next++) { + if (!vshDomainStatsPrintRecord(ctl, *next, raw)) + goto cleanup; + } + + ret = true; + cleanup: + virDomainStatsRecordListFree(records); + virDomainListFree(domlist); + + return ret; +} + const vshCmdDef domMonitoringCmds[] = { {.name = "domblkerror", .handler = cmdDomBlkError, @@ -2021,6 +2206,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, diff --git a/tools/virsh.pod b/tools/virsh.pod index e0dfd13..b08c938 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -813,6 +813,40 @@ that require a block device name (such as I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. +=item B<domstats> [[I<--list-active>] [I<--list-inactive>] +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>] +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domains>] +[I<--raw>] [I<--enforce>] [I<--state>] + +Get statistics for multiple or all domains. Without any argument this +command prints all available statistics for all domains. + +The list of domains to gather stats for can be either limited by listing +the domains as a space separated list, or by specifying one of the +filtering flags I<--list-*>. (The approaches can't be combined.) + +By default some of the returned fields may be converted to more +human friendly values by a set of pretty-printers. To suppress this +behavior use the I<--raw> flag. + +The individual statistics groups are selectable via specific flags. By +default all supported statistics groups are returned. Supported +statistics groups flags are: I<--state>. + +Selecting a specific statistics groups doesn't guarantee that the +daemon supports the selected group of stats. Flag I<--enforce> +forces the command to fail if the daemon doesn't support the +selected group. + + $ tools/virsh domstats --state dom1 dom2 + Domain: 'dom1' + state.state=shut off + state.reason=unknown + + Domain: 'dom2' + state.state=running + state.reason=booted + =item B<domiflist> I<domain> [I<--inactive>] Print a table showing the brief information of all virtual interfaces -- 2.0.2
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Add "domstats" command that excercises both of the new APIs depending if
s/excercises/exercises/
you specify a domain list or not. The output is printed as a key=value list of the returned parameters. --- tools/virsh-domain-monitor.c | 191 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 34 ++++++++ 2 files changed, 225 insertions(+)
+ +static bool +vshDomainStatsPrintRecord(vshControl *ctl ATTRIBUTE_UNUSED, + virDomainStatsRecordPtr record, + bool raw ATTRIBUTE_UNUSED) +{ + char *param; + size_t i; + + vshPrint(ctl, "Domain: '%s'\n", virDomainGetName(record->dom)); + + /* XXX: Implement pretty-printing */
Yeah, that can come in a later patch; this is already big enough to be a useful first round.
+ + VSH_EXCLUSIVE_OPTIONS("domains", "list-active"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-inactive"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-persistent"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-transient"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-running"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-paused"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-shutoff"); + VSH_EXCLUSIVE_OPTIONS("domains", "list-other");
Drop this hunk, and then you can validate that my tweak to 4/5 actually rejects combining things at the lower layer. Also, it is more future-proof - if we later change our minds, and DO allow filtering over a list of domain names, then we don't have to revisit the virsh command to get things to work. [1]...
+=item B<domstats> [[I<--list-active>] [I<--list-inactive>] +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>] +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domains>]
I'd list this as [I<domains>...], to make it obvious that more than one can be supported. Also, in most places where we have an ARGV option, we tend to name it in the singular: echo --arg, snapshot-create-as --diskspec, domfsfreeze --mountpoint. That has a ripple effect on the rest of your patch s/domains/domain/
+[I<--raw>] [I<--enforce>] [I<--state>] + +Get statistics for multiple or all domains. Without any argument this +command prints all available statistics for all domains. + +The list of domains to gather stats for can be either limited by listing +the domains as a space separated list, or by specifying one of the +filtering flags I<--list-*>. (The approaches can't be combined.)
...[1] But leave this comment in - we are just moving the enforcing to a lower layer of the stack, and letting virsh expose more power so that we have fuller testing over the lower layer behavior.
+ + $ tools/virsh domstats --state dom1 dom2
Drop 'tools/' from the example (even if it is what you pasted from your terminal session testing it).
+ Domain: 'dom1' + state.state=shut off + state.reason=unknown + + Domain: 'dom2' + state.state=running + state.reason=booted
Doesn't match the code (yet), so how about we drop this example until we have the followup patch for the pretty-printers. ACK with the tweaks above. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/28/14 00:15, Eric Blake wrote:
On 08/27/2014 12:25 PM, Peter Krempa wrote:
Add "domstats" command that excercises both of the new APIs depending if
s/excercises/exercises/
Hmmm, now that I've pushed this patch I noticed this typo :/ ... Oh well. Everybody knows that my English spelling is terrible :)
you specify a domain list or not. The output is printed as a key=value list of the returned parameters. --- tools/virsh-domain-monitor.c | 191 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 34 ++++++++ 2 files changed, 225 insertions(+)
...
ACK with the tweaks above.
I didn't forget to resolve the comments above before pushing. Thanks. peter
participants (2)
-
Eric Blake -
Peter Krempa