8:40 a.m.
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;