12:28 p.m.
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