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