| Message ID | 1450441425-10755-3-git-send-email-george.dunlap@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
George Dunlap writes ("[PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value"):
> 2. Use 'r' for return values to functions whose return values are a
> different error space (like xc_domain_setmaxmem and
> xc_domain_set_pod_target), or where a failure means retry, rather than
> fail the whole function (libxl__fill_dom0_memory_info), to reduce the
> risk that
The use of `r' to contain libxl__fill_dom0_memory_info's return value
is IMO confusing and contrary to CODING_STYLE.
Sorry that I didn't spot this last sentence in your point `2' when
reading ijc's review of your v3, where ijc suggested `r'.
> v4:
> - Use 'r' instead of 'lrc'
Can you go back to `lrc' for just that one use ? I think that would
be analogous with CODING_STYLE's suggestion to use `rc' for libxl
error codes.
> abort_transaction = 1;
> + rc = ERROR_FAIL;
There is an awful lot of this. I won't object to this in your patch,
as what you have is an improvement, but:
Every `goto out' here is preceded by `abort_transaction = 1'.
If you converted this function to:
- use libxl__xs_transaction_start et al
- in the intended pattern as shown in its doc comment
- use the `out' path only for errors
- unconditionally call libxl__xs_transaction_abort in the out
block
- call libxl__xs_transaction_commit in the success path
Then you could abolish `abort_transaction' and remove all assignments
to it. You could also abolish `out_no_transaction'.
If you were feeling really gung-ho you could get rid of `goto retry'
and replace it with a loop.
Ian.
On Mon, 2016-01-04 at 17:37 +0000, Ian Jackson wrote: > George Dunlap writes ("[PATCH v4 2/5] libxl: Fix libxl_set_memory_target > return value"): > > 2. Use 'r' for return values to functions whose return values are a > > different error space (like xc_domain_setmaxmem and > > xc_domain_set_pod_target), or where a failure means retry, rather than > > fail the whole function (libxl__fill_dom0_memory_info), to reduce the > > risk that > > The use of `r' to contain libxl__fill_dom0_memory_info's return value > is IMO confusing and contrary to CODING_STYLE. > > Sorry that I didn't spot this last sentence in your point `2' when > reading ijc's review of your v3, where ijc suggested `r'. And for my part sorry for not spotting the libxl_* in with the xc_*, or I'd have (hopefully) have thought to mention it. > > > v4: > > - Use 'r' instead of 'lrc' > > Can you go back to `lrc' for just that one use ? I think that would > be analogous with CODING_STYLE's suggestion to use `rc' for libxl > error codes. I was never quite sure what the "l" was supposed to reference, local? > > > > abort_transaction = 1; > > + rc = ERROR_FAIL; > > There is an awful lot of this. I won't object to this in your patch, > as what you have is an improvement, but: > > Every `goto out' here is preceded by `abort_transaction = 1'. > > If you converted this function to: > - use libxl__xs_transaction_start et al > - in the intended pattern as shown in its doc comment > - use the `out' path only for errors > - unconditionally call libxl__xs_transaction_abort in the out > block > - call libxl__xs_transaction_commit in the success path > > Then you could abolish `abort_transaction' and remove all assignments > to it. You could also abolish `out_no_transaction'. > > If you were feeling really gung-ho you could get rid of `goto retry' > and replace it with a loop. > > Ian.
Ian Campbell writes ("Re: [PATCH v4 2/5] libxl: Fix libxl_set_memory_target return value"):
> On Mon, 2016-01-04 at 17:37 +0000, Ian Jackson wrote:
> > Can you go back to `lrc' for just that one use ? I think that would
> > be analogous with CODING_STYLE's suggestion to use `rc' for libxl
> > error codes.
>
> I was never quite sure what the "l" was supposed to reference, local?
It seems to mean `local' to me. I was going to suggest `lrc' for this
variable name even before I saw that it had already been `lrc' in a
previous patch, which suggests it's a local maximum in probability
space...
Ian.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index a36101d..d05e58e 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -4742,7 +4742,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce) { GC_INIT(ctx); - int rc = 1, abort_transaction = 0; + int rc, abort_transaction = 0, r; uint64_t memorykb; uint32_t videoram = 0; uint32_t current_target_memkb = 0, new_target_memkb = 0; @@ -4770,15 +4770,15 @@ retry_transaction: if (!target && !domid) { if (!xs_transaction_end(ctx->xsh, t, 1)) goto out_no_transaction; - rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb, + r = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb, ¤t_max_memkb); - if (rc < 0) - goto out_no_transaction; + if (r < 0) { rc = ERROR_FAIL; goto out_no_transaction; } goto retry_transaction; } else if (!target) { LOGE(ERROR, "cannot get target memory info from %s/memory/target", dompath); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } else { current_target_memkb = strtoul(target, &endptr, 10); @@ -4786,6 +4786,7 @@ retry_transaction: LOGE(ERROR, "invalid memory target %s from %s/memory/target\n", target, dompath); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } } @@ -4794,6 +4795,7 @@ retry_transaction: LOGE(ERROR, "cannot get memory info from %s/memory/static-max", dompath); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } memorykb = strtoul(memmax, &endptr, 10); @@ -4801,6 +4803,7 @@ retry_transaction: LOGE(ERROR, "invalid max memory %s from %s/memory/static-max\n", memmax, dompath); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } @@ -4820,6 +4823,7 @@ retry_transaction: "memory_dynamic_max must be less than or equal to" " memory_static_max\n"); abort_transaction = 1; + rc = ERROR_INVAL; goto out; } @@ -4827,33 +4831,36 @@ retry_transaction: LOG(ERROR, "new target %d for dom0 is below the minimum threshold", new_target_memkb); abort_transaction = 1; + rc = ERROR_INVAL; goto out; } if (enforce) { memorykb = new_target_memkb + videoram; - rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb + + r = xc_domain_setmaxmem(ctx->xch, domid, memorykb + LIBXL_MAXMEM_CONSTANT); - if (rc != 0) { + if (r != 0) { LOGE(ERROR, "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, - rc); + r); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } } - rc = xc_domain_set_pod_target(ctx->xch, domid, + r = xc_domain_set_pod_target(ctx->xch, domid, (new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL); - if (rc != 0) { + if (r != 0) { LOGE(ERROR, "xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n", domid, new_target_memkb / 4, - rc); + r); abort_transaction = 1; + rc = ERROR_FAIL; goto out; } @@ -4867,6 +4874,8 @@ retry_transaction: "%"PRIu32, new_target_memkb / 1024); libxl_dominfo_dispose(&ptr); + rc = 0; + out: if (!xs_transaction_end(ctx->xsh, t, abort_transaction) && !abort_transaction)
libxl_set_memory_target seems to have the following return values: * 1 on failure, if the failure happens because of a xenstore error *or* invalid target * -1 if the setmaxmem hypercall * -errno if the set_pod_target hypercall target fails * 0 on success Make it consistently return ERROR_FAIL on failure, unless the parameters were invalid, in which case return ERROR_INVAL. In accordance with CODING_SYTLE: 1. Leave rc uninitialized, and set when an error is detected 2. Use 'r' for return values to functions whose return values are a different error space (like xc_domain_setmaxmem and xc_domain_set_pod_target), or where a failure means retry, rather than fail the whole function (libxl__fill_dom0_memory_info), to reduce the risk that Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- v4: - Use 'r' instead of 'lrc' - Set 'rc' before jumping to error path, rather than initializing at the beginning - Move removal of xc_domain_getinfolist to another patch CC: Ian Campbell <ian.campbell@citrix.com> CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/libxl.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)