Re: [libvirt] [PATCH v2 2/2] test_driver: implement virDomainGetCPUStats

On Sun, Jul 28, 2019 at 12:02:21PM +0200, Ilias Stamatis wrote: > Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; > --- > src/test/test_driver.c | 132 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 132 insertions(+) > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > index ab0f8b06d6..56f08fc3d2 100755 > --- a/src/test/test_driver.c > +++ b/src/test/test_driver.c > @@ -3629,6 +3629,137 @@ static int testDomainSetMetadata(virDomainPtr dom, > return ret; > } > > +#define TEST_TOTAL_CPUTIME 48772617035 Let's be explicit with ullong ^here by adding LL

> + > +static int > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params, > + int nparams) indent is off > +{ > + if (nparams == 0) /* return supported number of params */ > + return 3; > + > + if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME, > + VIR_TYPED_PARAM_ULLONG, TEST_TOTAL_CPUTIME) < 0) > + return -1; > + > + if (nparams > 1 && > + virTypedParameterAssign(&params[1], > + VIR_DOMAIN_CPU_STATS_USERTIME, > + VIR_TYPED_PARAM_ULLONG, 5540000000) < 0) > + return -1; > + > + if (nparams > 2 && > + virTypedParameterAssign(&params[2], > + VIR_DOMAIN_CPU_STATS_SYSTEMTIME, > + VIR_TYPED_PARAM_ULLONG, 6460000000) < 0) > + return -1; > + > + if (nparams > 3) > + nparams = 3; > + > + return nparams; > +} > + > + > +static int > +testDomainGetPercpuStats(virTypedParameterPtr params, > + unsigned int nparams, > + int start_cpu, > + unsigned int ncpus, > + int total_cpus) > +{ > + size_t i; > + int need_cpus; > + int param_idx; > + int ret = -1; @ret is unnecessary, see below > + > + /* return the number of supported params */ > + if (nparams == 0 && ncpus != 0) > + return 2; > + > + /* return total number of cpus */ > + if (ncpus == 0) { > + ret = total_cpus; > + goto cleanup; return total_cpus; > + } > + > + if (start_cpu >= total_cpus) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("start_cpu %d larger than maximum of %d"), > + start_cpu, total_cpus - 1); > + goto cleanup; return -1; > + } > + > + /* return percpu cputime in index 0 */ > + param_idx = 0; > + > + /* number of cpus to compute */ > + need_cpus = MIN(total_cpus, start_cpu + ncpus); > + > + for (i = 0; i < need_cpus; i++) { > + if (i < start_cpu) > + continue; How about initializing i = start_cpu straight away instead? > + int idx = (i - start_cpu) * nparams + param_idx; > + if (virTypedParameterAssign(&params[idx], > + VIR_DOMAIN_CPU_STATS_CPUTIME, > + VIR_TYPED_PARAM_ULLONG, > + (TEST_TOTAL_CPUTIME / total_cpus) + i) < 0) I'd strongly prefer if we didn't perform the division in each iteration,

the "+ i" also seems unnecessary.

> + goto cleanup; return -1; > + } > + > + /* return percpu vcputime in index 1 */ > + param_idx = 1; > + > + if (param_idx < nparams) { > + for (i = start_cpu; i < need_cpus; i++) { > + int idx = (i - start_cpu) * nparams + param_idx; > + if (virTypedParameterAssign(&params[idx], > + VIR_DOMAIN_CPU_STATS_VCPUTIME, > + VIR_TYPED_PARAM_ULLONG, > + (TEST_TOTAL_CPUTIME / total_cpus) - 1234567890 + i) < 0) Same as above... > + goto cleanup; return -1; > + } > + param_idx++; > + } > + > + ret = param_idx; return param_idx; > + cleanup: > + return ret; Drop the cleanup label.

> +} > + > + > +static int > +testDomainGetCPUStats(virDomainPtr dom, > + virTypedParameterPtr params, > + unsigned int nparams, > + int start_cpu, > + unsigned int ncpus, > + unsigned int flags) > +{ > + virDomainObjPtr vm = NULL; > + testDriverPtr privconn = dom->conn->privateData; > + int ret = -1; > + > + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); > + > + if (!(vm = testDomObjFromDomain(dom))) > + return -1; > + > + if (virDomainObjCheckActive(vm) < 0) > + goto cleanup; > + > + if (start_cpu == -1) > + ret = testDomainGetDomainTotalCpuStats(params, nparams); > + else > + ret = testDomainGetPercpuStats(params, nparams, start_cpu, ncpus, > + privconn->nodeInfo.cores); > + > + cleanup: > + virDomainObjEndAPI(&vm); > + return ret; > +} > + > + > static int > testDomainSendProcessSignal(virDomainPtr dom, > long long pid_value, > @@ -8552,6 +8683,7 @@ static virHypervisorDriver testHypervisorDriver = { > .domainSendKey = testDomainSendKey, /* 5.5.0 */ > .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */ > .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */ > + .domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */ > .domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */ > .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */ > .domainManagedSave = testDomainManagedSave, /* 1.1.4 */ > -- I'll adjust the code before merging.

Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;