[Xen-devel] [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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 <george.dunlap@xxxxxxxxxx>
---
CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
---
 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, &current_target_memkb,
+        lrc = libxl__fill_dom0_memory_info(gc, &current_target_memkb,
                                           &current_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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel