[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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


Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]