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