[PATCH v5] Fix xl vcpu-set to decrrease an guest vCPU amount without complaints.

[PATCH v5] Fix xl vcpu-set to decrrease an guest vCPU amount without complaints.

[Konrad Rzeszutek Wilk]

Hey Ian,

Since v4: [http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg03737.html]
 - Squashed libxl__check_max in libxl_set_vcpuonline
 - Added Acked-by

In v3 [http://lists.xen.org/archives/html/xen-devel/2015-03/msg02822.html]
- Constify the libxl_dominfo and handle libxl_domain_info errors.
- Made libxl_domain_info use logging if domain is gone.
- Drop libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

In v2 [http://lists.xen.org/archives/html/xen-devel/2015-03/msg01787.html]:
 - Fixed up #1 "libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info" per your review.
 - Moved the check from vcpuset to libxl_set_vcpuonline to check if the set bits of the
   cpumap is greater than the maximum vCPU that the guest can have (and added printks).
 - Added LIBXL_DOMAIN_TYPE_NOTFOUND so that libxl_set_vcpuonline can report that the
   domain id is for an non-existent guest.
 - Added an #define in libxl.h to expose the new _NOTFOUND error value.


All patches but one have been Acked. The patch:

 [PATCH v5 2/7] libxl: In libxl_set_vcpuonline check for maximum

has been reworked since the last posting.

The patches are also in:

 git://xenbits.xen.org/people/konradwilk/xen.git vcpuset.v5

Thank you!

 tools/libxl/libxl.c         | 66 +++++++++++++++++++++++++--------------------
 tools/libxl/libxl.h         |  9 ++++++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 47 ++++++++++++++++++++------------
 4 files changed, 76 insertions(+), 47 deletions(-)

Konrad Rzeszutek Wilk (7):
      libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain
      libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.
      libxl/libxl_domain_info: Log if domain not found.
      libxl/vcpuset: Print error if libxl_set_vcpuonline returns ERROR_DOMAIN_NOTFOUND
      libxl/vcpuset: Return error value if failed.
      libxl/vcpuset: Remove useless limit on max_vcpus.
      libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[PATCH v5 1/7] libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

[Konrad Rzeszutek Wilk]

And use that for all of its callers in the tree.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c         | 19 +++++++++++--------
 tools/libxl/libxl.h         |  9 ++++++++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  4 ++--
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a735b3..80c7ff6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -455,7 +455,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
     /* update /vm/<uuid>/name */
     rc = libxl_domain_info(ctx, &info, domid);
     if (rc)
-        goto x_fail;
+        goto x_rc;
 
     uuid = GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(info.uuid));
     vm_name_path = GCSPRINTF("/vm/%s/name", uuid);
@@ -698,7 +698,7 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
         return ERROR_FAIL;
     }
-    if (ret==0 || xcinfo.domain != domid) return ERROR_INVAL;
+    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
 
     if (info_r)
         xcinfo2xlinfo(ctx, &xcinfo, info_r);
@@ -1577,7 +1577,7 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     switch(rc) {
     case 0:
         break;
-    case ERROR_INVAL:
+    case ERROR_DOMAIN_NOTFOUND:
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "non-existant domain %d", domid);
     default:
         goto out;
@@ -5448,14 +5448,16 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
     libxl_dominfo info;
     char *dompath;
     xs_transaction_t t;
-    int i, rc = ERROR_FAIL;
+    int i, rc;
 
     libxl_dominfo_init(&info);
 
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
         LOGE(ERROR, "getting domain info list");
         goto out;
     }
+    rc = ERROR_FAIL;
     if (!(dompath = libxl__xs_get_dompath(gc, domid)))
         goto out;
 
@@ -5479,14 +5481,15 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
                                      libxl_bitmap *cpumap)
 {
     libxl_dominfo info;
-    int i;
+    int i, rc;
 
     libxl_dominfo_init(&info);
 
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
         LOGE(ERROR, "getting domain info list");
         libxl_dominfo_dispose(&info);
-        return ERROR_FAIL;
+        return rc;
     }
     for (i = 0; i <= info.vcpu_max_id; i++) {
         if (libxl_bitmap_test(cpumap, i)) {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bc75c5..1cf5699 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -179,6 +179,11 @@
 #define LIBXL_HAVE_BUILDINFO_HVM_MMIO_HOLE_MEMKB 1
 
 /*
+ * libxl_domain_info returns ERROR_DOMAIN_NOTFOUND if the domain
+ * is not present, instead of ERROR_INVAL.
+ */
+#define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -1104,7 +1109,9 @@ int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
  */
 int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
 
-/* May be called with info_r == NULL to check for domain's existance */
+/* May be called with info_r == NULL to check for domain's existence.
+ * Returns ERROR_DOMAIN_NOTFOUND if domain does not exist (used to return
+ * ERROR_INVAL for this scenario). */
 int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
                       uint32_t domid);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0866433..117b61d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -64,6 +64,7 @@ libxl_error = Enumeration("error", [
     (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
     (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
     (-20, "VNUMA_CONFIG_INVALID"),
+    (-21, "DOMAIN_NOTFOUND"),
     ], value_namespace = "")
 
 libxl_domain_type = Enumeration("domain_type", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 394b55d..1c07ac6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4736,7 +4736,7 @@ int main_list(int argc, char **argv)
     } else if (optind == argc-1) {
         uint32_t domid = find_domain(argv[optind]);
         rc = libxl_domain_info(ctx, &info_buf, domid);
-        if (rc == ERROR_INVAL) {
+        if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
@@ -5507,7 +5507,7 @@ int main_sharing(int argc, char **argv)
     } else if (optind == argc-1) {
         uint32_t domid = find_domain(argv[optind]);
         rc = libxl_domain_info(ctx, &info_buf, domid);
-        if (rc == ERROR_INVAL) {
+        if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
-- 
2.1.0

[PATCH v5 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Konrad Rzeszutek Wilk]

There is no sense in trying to online (or offline) CPUs when the size of
cpumap is greater than the maximum number of VCPUs the guest can go to.

As such fail the operation if the count of CPUs to online is greater
than what the guest started with. For the offline case we do not
check (as the bits are unset in the cpumap) and let it go through.

We coalesce some of the underlying libxl_set_vcpuonline code
together which was duplicated in QMP and XenStore codepaths.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
[v2: constify the libxl_domain and handle libxl_domain_info error]
[v3: inline libxl__check_max]
---
 tools/libxl/libxl.c | 63 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 80c7ff6..c0e9cfe 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5443,27 +5443,19 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
 }
 
 static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
-                                         libxl_bitmap *cpumap)
+                                         libxl_bitmap *cpumap,
+                                         const libxl_dominfo *info)
 {
-    libxl_dominfo info;
     char *dompath;
     xs_transaction_t t;
-    int i, rc;
-
-    libxl_dominfo_init(&info);
+    int i, rc = ERROR_FAIL;
 
-    rc = libxl_domain_info(CTX, &info, domid);
-    if (rc < 0) {
-        LOGE(ERROR, "getting domain info list");
-        goto out;
-    }
-    rc = ERROR_FAIL;
     if (!(dompath = libxl__xs_get_dompath(gc, domid)))
         goto out;
 
 retry_transaction:
     t = xs_transaction_start(CTX->xsh);
-    for (i = 0; i <= info.vcpu_max_id; i++)
+    for (i = 0; i <= info->vcpu_max_id; i++)
         libxl__xs_write(gc, t,
                        libxl__sprintf(gc, "%s/cpu/%u/availability", dompath, i),
                        "%s", libxl_bitmap_test(cpumap, i) ? "online" : "offline");
@@ -5473,25 +5465,16 @@ retry_transaction:
     } else
         rc = 0;
 out:
-    libxl_dominfo_dispose(&info);
     return rc;
 }
 
 static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
-                                     libxl_bitmap *cpumap)
+                                     libxl_bitmap *cpumap,
+                                     const libxl_dominfo *info)
 {
-    libxl_dominfo info;
-    int i, rc;
-
-    libxl_dominfo_init(&info);
+    int i;
 
-    rc = libxl_domain_info(CTX, &info, domid);
-    if (rc < 0) {
-        LOGE(ERROR, "getting domain info list");
-        libxl_dominfo_dispose(&info);
-        return rc;
-    }
-    for (i = 0; i <= info.vcpu_max_id; i++) {
+    for (i = 0; i <= info->vcpu_max_id; i++) {
         if (libxl_bitmap_test(cpumap, i)) {
             /* Return value is ignore because it does not tell anything useful
              * on the completion of the command.
@@ -5501,33 +5484,53 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
             libxl__qmp_cpu_add(gc, domid, i);
         }
     }
-    libxl_dominfo_dispose(&info);
     return 0;
 }
 
 int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
 {
     GC_INIT(ctx);
-    int rc;
+    int rc, maxcpus;
+    libxl_dominfo info;
+
+    libxl_dominfo_init(&info);
+
+    rc = libxl_domain_info(CTX, &info, domid);
+    if (rc < 0) {
+        LOGE(ERROR, "getting domain info list");
+        goto out;
+    }
+
+    maxcpus = libxl_bitmap_count_set(cpumap);
+    if (maxcpus > info.vcpu_max_id + 1)
+    {
+        LOGE(ERROR, "Requested %d VCPUs, however maxcpus is %d!",
+             maxcpus, info.vcpu_max_id + 1);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     switch (libxl__domain_type(gc, domid)) {
     case LIBXL_DOMAIN_TYPE_HVM:
         switch (libxl__device_model_version_running(gc, domid)) {
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-            rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+            rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap);
+            rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap, &info);
             break;
         default:
             rc = ERROR_INVAL;
         }
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+        rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
         break;
     default:
         rc = ERROR_INVAL;
     }
+out:
+    libxl_dominfo_dispose(&info);
     GC_FREE;
     return rc;
 }
-- 
2.1.0

Re: [PATCH v5 2/7] libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

[Ian Campbell]

On Fri, 2015-04-03 at 16:02 -0400, Konrad Rzeszutek Wilk wrote:
> There is no sense in trying to online (or offline) CPUs when the size of
> cpumap is greater than the maximum number of VCPUs the guest can go to.
> 
> As such fail the operation if the count of CPUs to online is greater
> than what the guest started with. For the offline case we do not
> check (as the bits are unset in the cpumap) and let it go through.
> 
> We coalesce some of the underlying libxl_set_vcpuonline code
> together which was duplicated in QMP and XenStore codepaths.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Konrad Rzeszutek Wilk]

If we cannot find the domain - log an error (and still
continue returning an error).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index c0e9cfe..8753e27 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -698,8 +698,10 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
         return ERROR_FAIL;
     }
-    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
-
+    if (ret==0 || xcinfo.domain != domid) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Domain %d not found!", domid);
+        return ERROR_DOMAIN_NOTFOUND;
+    }
     if (info_r)
         xcinfo2xlinfo(ctx, &xcinfo, info_r);
     return 0;
-- 
2.1.0

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Ian Murray]

On 03/04/15 21:02, Konrad Rzeszutek Wilk wrote:
> If we cannot find the domain - log an error (and still
> continue returning an error).
Forgive me if I am misunderstanding the effect of this patch (I tried to
find the original rationale but failed). If the effect is that commands
such as xl domid will cause a log entry when the specified domain
doesn't exist, I would suggest that's going to be a problem for people
that use that or similar commands to tell if a domain is present or
still alive. I use it as part of a back-up script to make sure a domain
shutdown before the script continues. I suspect many other people will
be doing something similar.

Apologies if I have the wrong end of the stick!

Thanks,

Ian.


>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index c0e9cfe..8753e27 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -698,8 +698,10 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
>          return ERROR_FAIL;
>      }
> -    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
> -
> +    if (ret==0 || xcinfo.domain != domid) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Domain %d not found!", domid);
> +        return ERROR_DOMAIN_NOTFOUND;
> +    }
>      if (info_r)
>          xcinfo2xlinfo(ctx, &xcinfo, info_r);
>      return 0;

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Konrad Rzeszutek Wilk]

On Fri, Apr 03, 2015 at 11:12:15PM +0100, Ian Murray wrote:
> On 03/04/15 21:02, Konrad Rzeszutek Wilk wrote:
> > If we cannot find the domain - log an error (and still
> > continue returning an error).
> Forgive me if I am misunderstanding the effect of this patch (I tried to
> find the original rationale but failed). If the effect is that commands
> such as xl domid will cause a log entry when the specified domain
> doesn't exist, I would suggest that's going to be a problem for people

It would.
> that use that or similar commands to tell if a domain is present or
> still alive. I use it as part of a back-up script to make sure a domain
> shutdown before the script continues. I suspect many other people will
> be doing something similar.

But won't 'xl domid' give you an return 0 if it exists and 1 if it does not?

Ah it does this (if it can't find the domain):

6195         fprintf(stderr, "Can't get domid of domain name '%s', maybe this domain does not exist.\n", domname);
6196         return 1;                                                               


If you are using 'xl list <domid>' it also prints:

4739         if (rc == ERROR_DOMAIN_NOTFOUND) {                                      
4740             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",           
4741                 argv[optind]);                                                  
4742             return -rc;                                  

(Previously it would also print this).

Either way the data is already presented to the user. With this
patch it is presented twice - which is repetitive.


Ian C, thoughts? Just ditch this patch? (The patchset can go in without
this one).

> 
> Apologies if I have the wrong end of the stick!

There is never an wrong end!
> 
> Thanks,
> 
> Ian.
> 
> 
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  tools/libxl/libxl.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index c0e9cfe..8753e27 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -698,8 +698,10 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
> >          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
> >          return ERROR_FAIL;
> >      }
> > -    if (ret==0 || xcinfo.domain != domid) return ERROR_DOMAIN_NOTFOUND;
> > -
> > +    if (ret==0 || xcinfo.domain != domid) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Domain %d not found!", domid);
> > +        return ERROR_DOMAIN_NOTFOUND;
> > +    }
> >      if (info_r)
> >          xcinfo2xlinfo(ctx, &xcinfo, info_r);
> >      return 0;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Ian Campbell]

On Mon, 2015-04-06 at 09:32 -0400, Konrad Rzeszutek Wilk wrote:
> Ian C, thoughts? Just ditch this patch?

I think so, my original suggestion to include the logging was predicated
on it not being usual for us to look up a non-existent domid, but the xl
domid usecase was one which didn't occur, and is a very valid reason not
to do this.

Alternatively could crank the log level all the way down to DEBUG, I'm
happy either way (at least until someone tells me why I'm not!).

Ian

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Ian Campbell]

On Wed, 2015-04-15 at 14:03 +0100, Ian Campbell wrote:
> On Mon, 2015-04-06 at 09:32 -0400, Konrad Rzeszutek Wilk wrote:
> > Ian C, thoughts? Just ditch this patch?
> 
> I think so, my original suggestion to include the logging was predicated
> on it not being usual for us to look up a non-existent domid, but the xl
> domid usecase was one which didn't occur, and is a very valid reason not
> to do this.
> 
> Alternatively could crank the log level all the way down to DEBUG, I'm
> happy either way (at least until someone tells me why I'm not!).

I realised that having acked #2 everything was acked, so I've applied
without this one, if you want to go the debug log route then please
could you send that separately?

Ian.

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Konrad Rzeszutek Wilk]

On Wed, Apr 15, 2015 at 02:30:58PM +0100, Ian Campbell wrote:
> On Wed, 2015-04-15 at 14:03 +0100, Ian Campbell wrote:
> > On Mon, 2015-04-06 at 09:32 -0400, Konrad Rzeszutek Wilk wrote:
> > > Ian C, thoughts? Just ditch this patch?
> > 
> > I think so, my original suggestion to include the logging was predicated
> > on it not being usual for us to look up a non-existent domid, but the xl
> > domid usecase was one which didn't occur, and is a very valid reason not
> > to do this.
> > 
> > Alternatively could crank the log level all the way down to DEBUG, I'm
> > happy either way (at least until someone tells me why I'm not!).
> 
> I realised that having acked #2 everything was acked, so I've applied
> without this one, if you want to go the debug log route then please
> could you send that separately?

Thank you for applying them!

Lets ignore the debug log route.
> 
> Ian.
> 

Re: [PATCH v5 3/7] libxl/libxl_domain_info: Log if domain not found.

[Konrad Rzeszutek Wilk]

On Wed, Apr 15, 2015 at 02:03:23PM +0100, Ian Campbell wrote:
> On Mon, 2015-04-06 at 09:32 -0400, Konrad Rzeszutek Wilk wrote:
> > Ian C, thoughts? Just ditch this patch?
> 
> I think so, my original suggestion to include the logging was predicated
> on it not being usual for us to look up a non-existent domid, but the xl
> domid usecase was one which didn't occur, and is a very valid reason not
> to do this.
> 
> Alternatively could crank the log level all the way down to DEBUG, I'm
> happy either way (at least until someone tells me why I'm not!).

Nah, lets ditch it.
> 
> Ian
> 

[PATCH v5 4/7] libxl/vcpuset: Print error if libxl_set_vcpuonline returns ERROR_DOMAIN_NOTFOUND

[Konrad Rzeszutek Wilk]

Instead of just printing an generic error.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1c07ac6..0ccf257 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5223,6 +5223,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     char *endptr;
     unsigned int max_vcpus, i;
     libxl_bitmap cpumap;
+    int rc;
 
     libxl_bitmap_init(&cpumap);
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
@@ -5253,8 +5254,12 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     for (i = 0; i < max_vcpus; i++)
         libxl_bitmap_set(&cpumap, i);
 
-    if (libxl_set_vcpuonline(ctx, domid, &cpumap) < 0)
-        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d\n", domid, max_vcpus);
+    rc = libxl_set_vcpuonline(ctx, domid, &cpumap);
+    if (rc == ERROR_DOMAIN_NOTFOUND)
+        fprintf(stderr, "Domain %u does not exist.\n", domid);
+    else if (rc)
+        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d," \
+                " rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
 }
-- 
2.1.0

[PATCH v5 5/7] libxl/vcpuset: Return error value if failed.

[Konrad Rzeszutek Wilk]

The function does not return any values at all. Convert the
internal libxl errors (ERROR_FAIL, ..., etc) to 1.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0ccf257..0a104ce 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5218,7 +5218,7 @@ int main_vcpupin(int argc, char **argv)
     return rc;
 }
 
-static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
+static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
 {
     char *endptr;
     unsigned int max_vcpus, i;
@@ -5229,7 +5229,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
     if (nr_vcpus == endptr) {
         fprintf(stderr, "Error: Invalid argument.\n");
-        return;
+        return 1;
     }
 
     /*
@@ -5242,14 +5242,15 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
             fprintf(stderr, "You are overcommmitting! You have %d physical " \
                     " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
                     " continue\n", host_cpu, max_vcpus);
-            return;
+            return 1;
         }
         /* NB: This also limits how many are set in the bitmap */
         max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
     }
-    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
-        fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
-        return;
+    rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
+    if (rc) {
+        fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n", rc);
+        return 1;
     }
     for (i = 0; i < max_vcpus; i++)
         libxl_bitmap_set(&cpumap, i);
@@ -5262,6 +5263,7 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                 " rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
+    return rc ? 1 : 0;
 }
 
 int main_vcpuset(int argc, char **argv)
@@ -5280,8 +5282,7 @@ int main_vcpuset(int argc, char **argv)
         break;
     }
 
-    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
-    return 0;
+    return vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
 }
 
 static void output_xeninfo(void)
-- 
2.1.0

[PATCH v5 6/7] libxl/vcpuset: Remove useless limit on max_vcpus.

[Konrad Rzeszutek Wilk]

The check is superflous. If the 'max_vcpus' (argument
value) is greater than  pCPU and --ignore-host has not
been supplied we would print an warning and return
and not call this code.

If the --ignore-host parameter had been used we would
never end up in this condition and enforce 'max_vcpus'.

The only time it would be invoked is if max_vcpus < host_cpu
in which case it would set max_vcpus to max_vcpus.

In short - it is dead code.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0a104ce..b121d75 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5244,8 +5244,6 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                     " continue\n", host_cpu, max_vcpus);
             return 1;
         }
-        /* NB: This also limits how many are set in the bitmap */
-        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
2.1.0

[PATCH v5 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v5)

[Konrad Rzeszutek Wilk]

We have a check to warn the user if they are overcommitting.
But the check only checks the hosts CPU amount and does
not take into account the case when the user is trying to fix
the overcommit. That is - they want to limit the amount of
online VCPUs.

This fix allows the user to offline vCPUs without any
warnings when they are running an overcommitted guest.

Also fix the extra space in the message.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
[v2: Remove crud code as spotted by Boris]
[v3: Moved crud code removal to another patch]
[v4: Remove the fprintf as it is done in libxl_domain_info]
[v5: Fix memory leak]
---
 tools/libxl/xl_cmdimpl.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b121d75..648ca08 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5238,12 +5238,21 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
      */
     if (check_host) {
         unsigned int host_cpu = libxl_get_max_cpus(ctx);
-        if (max_vcpus > host_cpu) {
-            fprintf(stderr, "You are overcommmitting! You have %d physical " \
-                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
-                    " continue\n", host_cpu, max_vcpus);
+        libxl_dominfo dominfo;
+
+        rc = libxl_domain_info(ctx, &dominfo, domid);
+        if (rc)
             return 1;
+
+        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
+            fprintf(stderr, "You are overcommmitting! You have %d physical" \
+                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
+                    " continue\n", host_cpu, max_vcpus);
+            rc = 1;
         }
+        libxl_dominfo_dispose(&dominfo);
+        if (rc)
+            return 1;
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
2.1.0