On 08/26/2014 08:14 AM, 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. > --- > src/qemu/qemu_driver.c | 175 > +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 175 insertions(+) >
This API should also be fairly easy to implement for the test:// driver, which would make it also easier to test how well it does. How hard would it be for you to copy the ideas of this patch into a patch 6? > > +static int > +qemuDomainGetStatsState(virDomainObjPtr dom, > + virDomainStatsRecordPtr record, > + int *maxparams, > + unsigned int privflags ATTRIBUTE_UNUSED) > + return -1; > + > + > + return 0; Why two blank lines? > +} > + > + > +typedef int > +(*virDomainGetStatsFunc)(virDomainObjPtr dom, > + virDomainStatsRecordPtr record, > + int *maxparams, > + unsigned int flags); > + > +struct virDomainGetStatsWorker { > + virDomainGetStatsFunc func; > + unsigned int stats; > +}; > + > +static struct virDomainGetStatsWorker virDomainGetStatsWorkers[] = { > + { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE}, > + { NULL, 0 } > +}; For example, the idea of virDomainGetStatsFunc as a callback may fit better under src/conf/domain* for easier sharing, while the specific implementation of the array belongs here in qemu_driver.c. > + > + > +static int > +virDomainGetStats(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; Is it necessary to malloc this, or can you just stack-allocate it and pass &tmp to other functions? > + > + for (i = 0; virDomainGetStatsWorkers[i].func; i++) { > + if (stats & VIR_DOMAIN_STATS_ALL || > + stats & virDomainGetStatsWorkers[i].stats) { Again, per my question on 1/5, I'm thinking the logic here might be better as: if (!stats || stats & virDomainGetStatsWorkers[i].stats) coupled with logic that says if stats has any bits that did not have at least one worker, but flags says to enforce all stats bits, then fail (actually, the logic of an impossible stats bit request can be hoisted to the caller - it is a one-time check outside the loop over all domains, while this is the code being done on the inner loop once per domain). > + > + if (stats & ~VIR_DOMAIN_STATS_ALL) { > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, > + _("Stats types bits 0x%x are not supported by this > daemon"), > + stats); > + goto cleanup; > + } As above, this check can be hoisted outside the outer loop of the caller (since neither the outer loop nor inner loop is changing the user's requested set of stats). > +static int > +qemuDomainListGetStats(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(0, -1); I think you want to call virDomainListGetStatsEnsureACL up front. Ouch - I see what's going on. The ACL generator thinks that because your API prefix is virDomain, that the ACL is per-domain, when in reality, you are doing a virConnect operation. I think you need to rename the driver.h callback in patch 1/5 so that the signature for the ACL check here becomes: virConnectDomainListGetStatsEnsureACL(conn) to prove whether we can even use this API, then later... > + > + if (!ndoms) { > + if ((ntempdoms = virDomainObjListExport(driver->domains, > + conn, > + &domlist, > + NULL, > + 0)) < 0) ...here, when gathering the list of domains you can iterate on, you want to pass the filter function virConnectDomainListGetStatsCheckACL, rather than a NULL filter. That way, your list omits domains that the user has no right to access, before wasting time collecting stats on those domains. > + goto cleanup; > + > + ndoms = ntempdoms; > + doms = domlist; > + } > + > + if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) > + goto cleanup; > + > + for (i = 0; i < ndoms; i++) { > + virDomainStatsRecordPtr tmp = NULL; > + > + if (!(dom = qemuDomObjFromDomain(doms[i]))) { > + if (domlist) > + continue; > + } > + > + if (!domlist && > + virDomainListGetStatsEnsureACL(conn, dom->def) < 0) > + continue; Hmm, here, if this is the global API, tmpstats should already have been filtered; but is this is the dom-list API, the user may have passed in a domain that they can list (virConnectListAllDomains only filters on domain:getattr) but not read (most stat functions filter on domain:read, which requires more permissions than domain:getattr). So maybe you need filtering here, too, but only when domlist is non-NULL. Maybe we should get Dan's review on this (since he wrote ACL). I'm fine if we commit to the API before freeze, even if we don't get the implementation reviewed for a couple more days - I'd rather get it right and not release a bug that could turn into a CVE for revealing too much information to an unprivileged user that should not have been able to read stats on a domain. > + cleanup: > + if (dom) > + virObjectUnlock(dom); > + Useless if. virObjectUnlock(NULL) is safe. (Hmm, why isn't syntax-check catching it?) > + if (tmpstats) > + virDomainStatsRecordListFree(tmpstats); Another useless if. We should make this function play nicely with a NULL argument. > + > + if (domlist) { Another useless if, since you took care to initialize ndoms = 0 and only increment it after you have a domlist. > + for (i = 0; i < ndoms; i++) > + virObjectUnref(domlist[i]); > + > + VIR_FREE(domlist); > + } > + > + return ret; > +} > + > + > static virDriver qemuDriver = { > .no = VIR_DRV_QEMU, > .name = QEMU_DRIVER_NAME, > @@ -17308,6 +17482,7 @@ static virDriver qemuDriver = { > .domainSetTime = qemuDomainSetTime, /* 1.2.5 */ > .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */ > .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* > 1.2.7 */ > + .domainListGetStats = qemuDomainListGetStats, /* 1.2.8 */ > }; > > -- 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