[libvirt] [PATCHv3 4/5] qemu: Implement bulk stats API and one of the stats groups to return

[Eric Blake]

On 08/27/2014 12:25 PM, Peter Krempa wrote:
> Implement the API function for virDomainListGetStats and
> virConnectGetAllDomainStats in a modular way and implement the
> VIR_DOMAIN_STATS_STATE group of statistics.
> 
> Although it may look like the function looks universal I'd rather not
> expose it to other drivers as the comming stats groups are likely to do

s/comming/coming/

> qemu specific stuff to obtain the stats.

And of course, we can always change our mind and refactor things later,
if it turns to be helpful.

> ---
>  src/qemu/qemu_driver.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 175 insertions(+)
> 

> +
> +static int
> +qemuDomainGetStats(virConnectPtr conn,
> +                   virDomainObjPtr dom,
> +                   unsigned int stats,
> +                   virDomainStatsRecordPtr *record,
> +                   unsigned int flags)
> +{
> +    int maxparams = 0;
> +    virDomainStatsRecordPtr tmp;
> +    size_t i;
> +    int ret = -1;
> +
> +    if (VIR_ALLOC(tmp) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
> +        if (stats == 0 ||
> +            stats & qemuDomainGetStatsWorkers[i].stats) {
> +            if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams,
> +                                                  flags) < 0)
> +                goto cleanup;
> +
> +            stats &= ~(stats & qemuDomainGetStatsWorkers[i].stats);

Ouch.  That doesn't quite work.  Consider if I have STATE=1, CPU=2, and
stat workers registered for both.  If the user passes in stats=STATE,
then on the first iteration, I see 'stats & statsworkers[0].stats' is
true, so I append state stats [*], then clear out the STATE bit.  On the
second iteration, I now see 'stats == 0' is true, so I append cpu stats,
even though the user didn't ask for them.

What I'd do is a pre-pass:
if (stats == 0) {
    for (i = 0; qemuDomainGetStatsWorkers[i].func; i++)
        stats |= qemuDomainGetStatsWorkers[i].stats;
}

then ditch the 'stats == 0' branch in the main loop.  That way, you are
always doing subtractive work on 'stats', without picking up the tail of
the workers.

Hmm, that said, subtractive work is STILL slightly risky - suppose that
I reach a point where it is easier to code two separate stat workers
that both provide STATE information (for whatever reason that combining
them into a single callback was too awkward - perhaps because in adding
other drivers, we find that some stats are easy to provide with a common
callback from src/conf, while other stats are still hypervisor-specific
but without duplicating the common code).  If I have statsWorkers[0] and
[1] both tied to STATE, then worker 1 will not be called because worker
0 removed STATE from stats.  On the other hand, it's still fairly
trivial to write the hypervisor-specific worker to call the common core
worker, and have only a single registration for STATE, so I'm probably
over-thinking this scenario.

> +        }
> +    }
> +
> +    if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS &&
> +        stats != 0) {
> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> +                       _("Stats types bits 0x%x are not supported by this daemon"),
> +                       stats);
> +        goto cleanup;
> +    }

Even better, this 'if' can fail at most once (on the first domain being
listed); if the first domain succeeded, you have wasted a conditional on
every subsequent domain.  You could still hoist the stats pre-pass
entirely to the caller, and not even worry about manipulating 'stats' in
this call.

> +static int
> +qemuConnectGetAllDomainStats(virConnectPtr conn,
> +                             virDomainPtr *doms,
> +                             unsigned int ndoms,
> +                             unsigned int stats,
> +                             virDomainStatsRecordPtr **retStats,
> +                             unsigned int flags)
> +{
> +    virQEMUDriverPtr driver = conn->privateData;
> +    virDomainPtr *domlist = NULL;
> +    virDomainObjPtr dom = NULL;
> +    virDomainStatsRecordPtr *tmpstats = NULL;
> +    int ntempdoms;
> +    int nstats = 0;
> +    size_t i;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE |
> +                  VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);

As promised in the review of 2/5, I think you could do:

/* Allow filter flags only when called via virConnect variant */
if (ndoms)
    virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1)
else
    virCheckFlags(all the bits, -1)

so that you forcefully reject callers from trying to do filtering when
using the domlist variant of the public API.  That way, if we later
change our mind, and decide to let filtering work, users are guaranteed
sane behavior (change from a hard error to working is better than change
from silently ignored to working).

I think this one needs a v4, just to make sure we get the modularity
right when there is more than one stats bit to support.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140827/40e425ab/attachment-0001.sig>