[libvirt] [PATCH 2/3] lxc: factorize code for block stats
John Ferlan
jferlan at redhat.com
Wed Mar 7 14:40:23 UTC 2018
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;
>
More information about the libvir-list
mailing list