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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list