[PATCH v3] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[PATCH v3] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Stefano Stabellini]

In libxl_set_memory_target when setting the new maxmem, retain the same
offset on top of the current target. In the future the offset will
include memory allocated by QEMU for rom files.

Given that LIBXL_MAXMEM_CONSTANT is already added to maxmem at VM
creation time, we don't need to add it again here.

Error out if new_target_memkb <= 0.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v3:
- move call to libxl__uuid2string and libxl_dominfo_dispose earlier;
- error out if new_target_memkb <= 0.

Changes in v2:
- remove LIBXL_MAXMEM_CONSTANT from LIBXL__LOG_ERRNO.
---
 tools/libxl/libxl.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd6f42c..4f093f2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4717,6 +4717,12 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     char *uuid;
     xs_transaction_t t;
 
+    if (libxl_domain_info(ctx, &ptr, domid) < 0)
+        goto out_no_transaction;
+
+    uuid = libxl__uuid2string(gc, ptr.uuid);
+    libxl_dominfo_dispose(&ptr);
+
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
 
@@ -4775,6 +4781,14 @@ retry_transaction:
             new_target_memkb = current_target_memkb + target_memkb;
     } else
         new_target_memkb = target_memkb - videoram;
+
+    if (new_target_memkb <= 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                "cannot set memory target to 0 or less than 0.\n");
+        abort_transaction = 1;
+        goto out;
+    }
+
     if (new_target_memkb > memorykb) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                 "memory_dynamic_max must be less than or equal to"
@@ -4792,13 +4806,12 @@ retry_transaction:
     }
 
     if (enforce) {
-        memorykb = new_target_memkb + videoram;
-        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
-                LIBXL_MAXMEM_CONSTANT);
+        memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;
+        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                     "xc_domain_setmaxmem domid=%d memkb=%d failed "
-                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc);
+                    "rc=%d\n", domid, memorykb, rc);
             abort_transaction = 1;
             goto out;
         }
@@ -4823,12 +4836,8 @@ retry_transaction:
         goto out;
     }
 
-    libxl_dominfo_init(&ptr);
-    xcinfo2xlinfo(ctx, &info, &ptr);
-    uuid = libxl__uuid2string(gc, ptr.uuid);
     libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
             "%"PRIu32, new_target_memkb / 1024);
-    libxl_dominfo_dispose(&ptr);
 
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
-- 
1.7.10.4

Re: [PATCH v3] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Ian Campbell]

On Thu, 2015-01-29 at 15:08 +0000, Stefano Stabellini wrote:
> @@ -4775,6 +4781,14 @@ retry_transaction:
>              new_target_memkb = current_target_memkb + target_memkb;
>      } else
>          new_target_memkb = target_memkb - videoram;
> +
> +    if (new_target_memkb <= 0) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                "cannot set memory target to 0 or less than 0.\n");


new_target_memkb is uint32 so it can't be less than zero.

In fact, there looks to be some under/overflow bugs hidden in this
function. e.g. in "new_target_memkb = target_memkb" (since target is
signed)

Ian.

Re: [PATCH v3] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Stefano Stabellini]

On Mon, 2 Feb 2015, Ian Campbell wrote:
> On Thu, 2015-01-29 at 15:08 +0000, Stefano Stabellini wrote:
> > @@ -4775,6 +4781,14 @@ retry_transaction:
> >              new_target_memkb = current_target_memkb + target_memkb;
> >      } else
> >          new_target_memkb = target_memkb - videoram;
> > +
> > +    if (new_target_memkb <= 0) {
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > +                "cannot set memory target to 0 or less than 0.\n");
> 
> 
> new_target_memkb is uint32 so it can't be less than zero.

I'll remove the check


> In fact, there looks to be some under/overflow bugs hidden in this
> function. e.g. in "new_target_memkb = target_memkb" (since target is
> signed)

That assignment is only done when !relative, in that case target_memkb
should be a positive integer.