Re: [libvirt] [PATCHv2 3/5] qemu: Implement bulk stats API and one of the stats groups to return

On 08/26/2014 08:14 AM, Peter Krempa wrote: 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? Why two blank lines? 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. Is it necessary to malloc this, or can you just stack-allocate it and pass &tmp to other functions? 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). 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). 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... ...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. 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. Useless if. virObjectUnlock(NULL) is safe. (Hmm, why isn't syntax-check catching it?) Another useless if. We should make this function play nicely with a NULL argument. Another useless if, since you took care to initialize ndoms = 0 and only increment it after you have a domlist. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org