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