From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value Date: Tue, 1 Dec 2015 11:53:51 +0000 Message-ID: <1448970835-2706-2-git-send-email-george.dunlap@eu.citrix.com> References: <1448970835-2706-1-git-send-email-george.dunlap@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1448970835-2706-1-git-send-email-george.dunlap@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: George Dunlap , Ian Jackson , Wei Liu , George Dunlap , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 * 1 on success (!) Make it consistenstly return ERROR_FAIL, unless the parameters were invalid. To make this more robust, use 'lrc' 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 future code shuffles will accidentally clobber the return value again. Also remove the final call to xc_domain_getinfolist. There's no obvious reason for this call -- all it seems to be doing is checking to see if the domain exists; but if it doesn't exist, it will have already failed by this point. Signed-off-by: George Dunlap --- CC: Ian Campbell CC: Ian Jackson CC: Wei Liu --- tools/libxl/libxl.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 814d056..f8a0642 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -4722,7 +4722,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 = ERROR_FAIL, abort_transaction = 0, lrc; uint64_t memorykb; uint32_t videoram = 0; uint32_t current_target_memkb = 0, new_target_memkb = 0; @@ -4750,9 +4750,9 @@ 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, + lrc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb, ¤t_max_memkb); - if (rc < 0) + if (lrc < 0) goto out_no_transaction; goto retry_transaction; } else if (!target) { @@ -4800,6 +4800,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; } @@ -4807,43 +4808,39 @@ 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 + + lrc = xc_domain_setmaxmem(ctx->xch, domid, memorykb + LIBXL_MAXMEM_CONSTANT); - if (rc != 0) { + if (lrc != 0) { LOGE(ERROR, "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed ""rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, - rc); + lrc); abort_transaction = 1; goto out; } } - rc = xc_domain_set_pod_target(ctx->xch, domid, + lrc = xc_domain_set_pod_target(ctx->xch, domid, (new_target_memkb + LIBXL_MAXMEM_CONSTANT) / 4, NULL, NULL, NULL); - if (rc != 0) { + if (lrc != 0) { LOGE(ERROR, "xc_domain_set_pod_target domid=%d, memkb=%d ""failed rc=%d\n", domid, new_target_memkb / 4, - rc); + lrc); abort_transaction = 1; goto out; } libxl__xs_write(gc, t, GCSPRINTF("%s/memory/target", dompath), "%"PRIu32, new_target_memkb); - rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info); - if (rc != 1 || info.domain != domid) { - abort_transaction = 1; - goto out; - } libxl_dominfo_init(&ptr); xcinfo2xlinfo(ctx, &info, &ptr); @@ -4852,6 +4849,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) -- 2.1.4