s/factorize/Refactor/ On 03/05/2018 12:21 PM, Cédric Bosdonnat wrote:

lxcDomainBlockStats and lxcDomainBlockStatsFlags were both using very similar code. This commit factorizes them into a

s/factorsizes/refactors

lxcDomainBlockStatsInternal. --- src/lxc/lxc_driver.c | 131 ++++++++++++++++++++------------------------------- 1 file changed, 50 insertions(+), 81 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fa6fc4643..a16dbcc96 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2348,28 +2348,22 @@ lxcDomainMergeBlkioDevice(virBlkioDevicePtr *dest_array, return 0; }

Two empty lines between functions

- -static int -lxcDomainBlockStats(virDomainPtr dom, - const char *path, - virDomainBlockStatsPtr stats) +static int lxcDomainBlockStatsInternal(virLXCDriverPtr driver, + virDomainObjPtr vm, + const char *path, + long long *rd_bytes, + long long *wr_bytes, + long long *rd_req, + long long *wr_req)

Preference is static int lxcDomainBlockStatsInternal(args) The downside of passing 4 args is that sometime in the future there's 4 more stats to collect, then 4 more... I think you could just pass the stats pointer here

{ - virLXCDriverPtr driver = dom->conn->privateData; int ret = -1; - virDomainObjPtr vm; virDomainDiskDefPtr disk = NULL; virLXCDomainObjPrivatePtr priv;

- if (!(vm = lxcDomObjFromDomain(dom))) - return ret; - priv = vm->privateData;

- if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_QUERY) < 0) - goto cleanup; + return -1;

if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2386,10 +2380,10 @@ lxcDomainBlockStats(virDomainPtr dom, if (!*path) { /* empty path - return entire domain blkstats instead */ ret = virCgroupGetBlkioIoServiced(priv->cgroup, - &stats->rd_bytes, - &stats->wr_bytes, - &stats->rd_req, - &stats->wr_req); + rd_bytes, + wr_bytes, + rd_req, + wr_req);

Existing, but if ret < 0, then we return with a generic failed for some unknown reason. You could add the message from lxcDomainBlockStatsFlags of _("domain stats query failed") if (ret < 0) Probably would make the following goto stand out or you could use the if {} else {} like lxcDomainBlockStatsFlags does. That's your call.

goto endjob; }

@@ -2407,13 +2401,36 @@ lxcDomainBlockStats(virDomainPtr dom,

ret = virCgroupGetBlkioIoDeviceServiced(priv->cgroup, disk->info.alias, - &stats->rd_bytes, - &stats->wr_bytes, - &stats->rd_req, - &stats->wr_req); + rd_bytes, + wr_bytes, + rd_req, + wr_req);

endjob: virLXCDomainObjEndJob(driver, vm); + return ret; +} +

2 empty lines

+static int +lxcDomainBlockStats(virDomainPtr dom, + const char *path, + virDomainBlockStatsPtr stats) +{ + virLXCDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + if (!(vm = lxcDomObjFromDomain(dom))) + return -1; + + if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + ret = lxcDomainBlockStatsInternal(driver, vm, path, + &stats->rd_bytes, + &stats->wr_bytes, + &stats->rd_req, + &stats->wr_req);

cleanup: virDomainObjEndAPI(&vm); @@ -2431,8 +2448,6 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,

Could we take the opportunity to "alter" the "const char * path" to be "const char *path"... Same for "int * nparams".

virLXCDriverPtr driver = dom->conn->privateData; int tmp, ret = -1; virDomainObjPtr vm; - virDomainDiskDefPtr disk = NULL; - virLXCDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes;

Change these to be: virDomainBlockStats stats = { 0 }; and pass &stats to lxcDomainBlockStatsInternal and use the stats.* values results that you want to use in the virTypedParameterAssign calls

virTypedParameterPtr param;

@@ -2449,60 +2464,17 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, if (!(vm = lxcDomObjFromDomain(dom))) return ret;

- priv = vm->privateData; - if (virDomainBlockStatsFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;

- if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_QUERY) < 0) + if (lxcDomainBlockStatsInternal(driver, vm, path, + &rd_bytes, + &wr_bytes, + &rd_req, + &wr_req) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("domain stats query failed"));

This will overwrite what lxcDomainBlockStatsInternal had and return this message. As noted above, perhaps this message needs to move into the *Internal function and be displayed when virCgroupGetBlkioIoServiced fails. John

goto cleanup; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } - - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_BLKIO)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("blkio cgroup isn't mounted")); - goto endjob; - } - - if (!*path) { - /* empty path - return entire domain blkstats instead */ - if (virCgroupGetBlkioIoServiced(priv->cgroup, - &rd_bytes, - &wr_bytes, - &rd_req, - &wr_req) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("domain stats query failed")); - goto endjob; - } - } else { - if (!(disk = virDomainDiskByName(vm->def, path, false))) { - virReportError(VIR_ERR_INVALID_ARG, - _("invalid path: %s"), path); - goto endjob; - } - - if (!disk->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); - goto endjob; - } - - if (virCgroupGetBlkioIoDeviceServiced(priv->cgroup, - disk->info.alias, - &rd_bytes, - &wr_bytes, - &rd_req, - &wr_req) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("domain stats query failed")); - goto endjob; - } }

tmp = 0; @@ -2512,7 +2484,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, param = ¶ms[tmp]; if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, VIR_TYPED_PARAM_LLONG, wr_bytes) < 0) - goto endjob; + goto cleanup; tmp++; }

@@ -2520,7 +2492,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, param = ¶ms[tmp]; if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, VIR_TYPED_PARAM_LLONG, wr_req) < 0) - goto endjob; + goto cleanup; tmp++; }

@@ -2528,7 +2500,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, param = ¶ms[tmp]; if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_BYTES, VIR_TYPED_PARAM_LLONG, rd_bytes) < 0) - goto endjob; + goto cleanup; tmp++; }

@@ -2536,16 +2508,13 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, param = ¶ms[tmp]; if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_REQ, VIR_TYPED_PARAM_LLONG, rd_req) < 0) - goto endjob; + goto cleanup; tmp++; }

ret = 0; *nparams = tmp;

- endjob: - virLXCDomainObjEndJob(driver, vm); - cleanup: virDomainObjEndAPI(&vm); return ret;