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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
