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; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list