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

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

[Konrad Rzeszutek Wilk]

Hey Ian,

Since 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.

In short, these patches:
 [PATCH v3 1/7] libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info
 [PATCH v3 2/7] libxl: Add to libxl__domain_type a new return value
 [PATCH v3 3/7] libxl: In libxl_set_vcpuonline check for maximum
 [PATCH v3 4/7] libxl/vcpuset: Print error if libxl_set_vcpuonline

are new. While these have been redone since v2 review:
 [PATCH v3 5/7] libxl/vcpuset: Return error value if failed.
 [PATCH v3 7/7] libxl/vcpu-set - allow to decrease vcpu count on

And this one is Acked and has not changed since v2 posting:
 [PATCH v3 6/7] libxl/vcpuset: Remove useless limit on max_vcpus.

These patches are also available at:
 
 git://xenbits.xen.org/people/konradwilk/xen.git vcpuset.v3


Thank you for taking the time to review these patches!
 
 tools/libxl/libxl.c         | 97 ++++++++++++++++++++++++++++++---------------
 tools/libxl/libxl.h         | 14 ++++++-
 tools/libxl/libxl_dom.c     |  5 ++-
 tools/libxl/libxl_pci.c     |  6 +++
 tools/libxl/libxl_types.idl |  3 ++
 tools/libxl/xl_cmdimpl.c    | 47 ++++++++++++++--------
 6 files changed, 121 insertions(+), 51 deletions(-)

[PATCH v3 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>
---
 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 46de4f7..0b57bae 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;
@@ -5449,14 +5449,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;
 
@@ -5480,14 +5482,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 v3 2/7] libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

[Konrad Rzeszutek Wilk]

So that the callers can distinguish between an error and
an domain not found. The exposed API calls that are effected
by this are: libxl_domain_[remus_start,suspend,unpause,cdrom_insert,
set_vcpuonline]

We add an helper function to deal with the two types of errors:
libxl_domain_type2err. However for libxl_[pci,dom].c we just add
the extra check for LIBXL_DOMAIN_TYPE_NOTFOUND for simplicity
reasons.

Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c         | 53 ++++++++++++++++++++++++++-------------------
 tools/libxl/libxl.h         |  5 +++++
 tools/libxl/libxl_dom.c     |  5 ++++-
 tools/libxl/libxl_pci.c     |  6 +++++
 tools/libxl/libxl_types.idl |  2 ++
 5 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0b57bae..4152ee4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -354,6 +354,16 @@ const char *libxl_defbool_to_string(libxl_defbool b)
         return LIBXL__DEFBOOL_STR_DEFAULT;
 }
 
+static int libxl_domain_type2error(libxl_domain_type type)
+{
+    switch (type) {
+    case LIBXL_DOMAIN_TYPE_INVALID: return ERROR_FAIL;
+    case LIBXL_DOMAIN_TYPE_NOTFOUND: return ERROR_DOMAIN_NOTFOUND;
+    default: break;
+    }
+    return 0;
+}
+
 /******************************************************************************/
 
 
@@ -512,7 +522,7 @@ int libxl_domain_rename(libxl_ctx *ctx, uint32_t domid,
 
 int libxl__domain_resume(libxl__gc *gc, uint32_t domid, int suspend_cancel)
 {
-    int rc = 0;
+    int rc;
 
     if (xc_domain_resume(CTX->xch, domid, suspend_cancel)) {
         LOGE(ERROR, "xc_domain_resume failed for domain %u", domid);
@@ -521,10 +531,9 @@ int libxl__domain_resume(libxl__gc *gc, uint32_t domid, int suspend_cancel)
     }
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out;
-    }
 
     if (type == LIBXL_DOMAIN_TYPE_HVM) {
         rc = libxl__domain_resume_device_model(gc, domid);
@@ -842,10 +851,9 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
     int rc;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out;
-    }
 
     libxl_defbool_setdefault(&info->allow_unsafe, false);
     libxl_defbool_setdefault(&info->blackhole, false);
@@ -961,10 +969,9 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags,
     int rc;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out_err;
-    }
 
     libxl__domain_suspend_state *dss;
     GCNEW(dss);
@@ -1024,13 +1031,12 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
     GC_INIT(ctx);
     char *path;
     char *state;
-    int ret, rc = 0;
+    int ret, rc;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out;
-    }
 
     if (type == LIBXL_DOMAIN_TYPE_HVM) {
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
@@ -1059,8 +1065,9 @@ int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
     int ret;
 
     libxl_domain_type domtype = libxl__domain_type(gc, domid);
-    if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
-        return ERROR_FAIL;
+    ret = libxl_domain_type2error(domtype);
+    if (ret)
+        return ret;
 
     if (domtype == LIBXL_DOMAIN_TYPE_PV)
         return 1;
@@ -2390,10 +2397,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
     libxl_device_disk_copy(ctx, &disk_saved, disk);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out;
-    }
 
     /*
      * get_vdev != NULL -> local attach
@@ -2840,10 +2846,10 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     libxl__device_disk_setdefault(gc, &disk_empty);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    rc = libxl_domain_type2error(type);
+    if (rc)
         goto out;
-    }
+
     if (type != LIBXL_DOMAIN_TYPE_HVM) {
         LOG(ERROR, "cdrom-insert requires an HVM domain");
         rc = ERROR_INVAL;
@@ -5526,6 +5532,9 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
     case LIBXL_DOMAIN_TYPE_PV:
         rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
         break;
+    case LIBXL_DOMAIN_TYPE_NOTFOUND:
+        rc = ERROR_DOMAIN_NOTFOUND;
+        break;
     default:
         rc = ERROR_INVAL;
     }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1cf5699..f03fa3b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -181,6 +181,11 @@
 /*
  * libxl_domain_info returns ERROR_DOMAIN_NOTFOUND if the domain
  * is not present, instead of ERROR_INVAL.
+ *
+ * libxl_set_vcpuonline, libxl_domain_remus_start, libx_domain_suspend,
+ * libxl_domain_unpause, and libxl_cdrom_insert can return
+ * LIBLX_DOMAIN_TYPE_NOTFOUND if the domain is not present, instead of
+ * LIBXL_DOMAIN_TYPE_INVALID.
  */
 #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
 /*
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index ace8a66..1d11729 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -32,10 +32,13 @@ libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
     int ret;
 
     ret = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
-    if (ret != 1 || info.domain != domid) {
+    if (ret != 1) {
         LOG(ERROR, "unable to get domain type for domid=%"PRIu32, domid);
         return LIBXL_DOMAIN_TYPE_INVALID;
     }
+    if (info.domain != domid)
+        return LIBXL_DOMAIN_TYPE_NOTFOUND;
+
     if (info.flags & XEN_DOMINF_hvm_guest)
         return LIBXL_DOMAIN_TYPE_HVM;
     else
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..958c1a3 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -143,6 +143,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;
 
+    if (domtype == LIBXL_DOMAIN_TYPE_NOTFOUND)
+        return ERROR_DOMAIN_NOTFOUND;
+
     if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0)
             return ERROR_FAIL;
@@ -212,6 +215,9 @@ static int libxl__device_pci_remove_xenstore(libxl__gc *gc, uint32_t domid, libx
     if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;
 
+    if (domtype == LIBXL_DOMAIN_TYPE_NOTFOUND)
+        return ERROR_DOMAIN_NOTFOUND;
+
     if (domtype == LIBXL_DOMAIN_TYPE_PV) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "pci backend at %s is not ready", be_path);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 117b61d..d81e0c2 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -69,6 +69,7 @@ libxl_error = Enumeration("error", [
 
 libxl_domain_type = Enumeration("domain_type", [
     (-1, "INVALID"),
+    (-2, "NOTFOUND"),
     (1, "HVM"),
     (2, "PV"),
     ], init_val = "LIBXL_DOMAIN_TYPE_INVALID")
@@ -469,6 +470,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                       ("e820_host", libxl_defbool),
                                       ])),
                  ("invalid", None),
+                 ("notfound", None),
                  ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
     ], dir=DIR_IN
 )
-- 
2.1.0

[PATCH v3 3/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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4152ee4..d2b5ff3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5449,6 +5449,20 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
     return 0;
 }
 
+static int libxl__check_max(libxl__gc *gc, libxl_dominfo *info,
+                            libxl_bitmap *cpumap)
+{
+    int maxcpus = libxl_bitmap_count_set(cpumap);
+
+    if (maxcpus > info->vcpu_max_id + 1)
+    {
+        LOGE(ERROR, "You have a max of %d vCPUs and you want %d vCPUs!",
+             info->vcpu_max_id + 1, maxcpus);
+        return ERROR_FAIL;
+    }
+    return 0;
+}
+
 static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
                                          libxl_bitmap *cpumap)
 {
@@ -5464,6 +5478,9 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "getting domain info list");
         goto out;
     }
+    rc = libxl__check_max(gc, &info, cpumap);
+    if (rc)
+        goto out;
     rc = ERROR_FAIL;
     if (!(dompath = libxl__xs_get_dompath(gc, domid)))
         goto out;
@@ -5495,9 +5512,11 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
     rc = libxl_domain_info(CTX, &info, domid);
     if (rc < 0) {
         LOGE(ERROR, "getting domain info list");
-        libxl_dominfo_dispose(&info);
-        return rc;
+        goto out;
     }
+    rc = libxl__check_max(gc, &info, cpumap);
+    if (rc)
+        goto out;
     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
@@ -5508,8 +5527,10 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
             libxl__qmp_cpu_add(gc, domid, i);
         }
     }
+    rc = 0;
+out:
     libxl_dominfo_dispose(&info);
-    return 0;
+    return rc;
 }
 
 int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
-- 
2.1.0

[PATCH v3 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>
---
 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 v3 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>
---
 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 v3 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 v3 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)

[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>

---
[v2: Remove crud code as spotted by Boris]
[v3: Moved crud code removal to another patch]
---
 tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b121d75..d7bd5d5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5238,9 +5238,18 @@ 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 " \
+        libxl_dominfo dominfo;
+
+        rc = libxl_domain_info(ctx, &dominfo, domid);
+        if (rc)
+        {
+             if (rc == ERROR_DOMAIN_NOTFOUND)
+                fprintf(stderr, "Domain %u does not exist.\n", domid);
+            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);
             return 1;
         }
-- 
2.1.0

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

[Ian Campbell]

Please remember to CC all the relevant maintainers, e.g. Wei and Ian in
addition to me. MAINTAINERS is helpful here.

On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

although I started a conversation in response to a similar patch from
Ian J regarding the semantics (I cc-d you).

> ---
>  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 46de4f7..0b57bae 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;
> @@ -5449,14 +5449,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;
>  
> @@ -5480,14 +5482,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;

Re: [PATCH v3 2/7] libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

[Ian Campbell]

On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> So that the callers can distinguish between an error and
> an domain not found. The exposed API calls that are effected
> by this are: libxl_domain_[remus_start,suspend,unpause,cdrom_insert,
> set_vcpuonline]
> 
> We add an helper function to deal with the two types of errors:
> libxl_domain_type2err. However for libxl_[pci,dom].c we just add
> the extra check for LIBXL_DOMAIN_TYPE_NOTFOUND for simplicity
> reasons.
> 
> Suggested-by: Ian Campbell <ian.campbell@citrix.com>

Did I?

Anyway, I'm not terribly keen on the approach taken here, sorry, in
particular extending libxl_domain_type with a subset of the libxl_error
codes and the shenanigans which are needed to convert between them.

Off hand I can think of 3 possible alternative solutions:

Arrange that the -ve error values in libxl_domain_type directly
correspond to an appropriate libxl_error code, allowing code to do
    if (type < 0) return type;
    if (type < 0) { rc = type ; goto out }
type stuff.

Change the prototype of libxl__domain_type to always return a libxl rc
and on success return the type via a pointer argument.

Change the return type of libxl_domain_type to an int and return either
a LIBXL_DOMAIN_TYPE_FOO or an ERROR_*.

They all have downsides (I don't much like the implicit cast between
domain type and error code, changing the prototype will probably be
disruptive, changing the return type means gcc can't catch missing
switch statements for us).

Perhaps you or my fellow maintainers have a better idea or a preference
for one of the above?

Ian.

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

[Ian Campbell]

On Mon, 2015-03-23 at 14:20 -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.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 4152ee4..d2b5ff3 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5449,6 +5449,20 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
>      return 0;
>  }
>  
> +static int libxl__check_max(libxl__gc *gc, libxl_dominfo *info,
> +                            libxl_bitmap *cpumap)
> +{
> +    int maxcpus = libxl_bitmap_count_set(cpumap);
> +
> +    if (maxcpus > info->vcpu_max_id + 1)
> +    {
> +        LOGE(ERROR, "You have a max of %d vCPUs and you want %d vCPUs!",
> +             info->vcpu_max_id + 1, maxcpus);

Please avoid personal pronouns in libxl logs (I don't have a max of any
number of vcpus, the domain does). Here "setting %d vcpus, max vcpus is
%d" perhaps.

Both libxl__set_vcpuonline_qmp and libxl__set_vcpuonline_xenstore start
with the same code to get info. Please could you move that (and the
associated dispose) into libxl_set_vcpuonline and check the limit once
up front (no need for helper then) and pass the info as a pointer to the
specific functions.

Ian.

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

[Ian Campbell]

On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

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

[Ian Campbell]

On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> The function does not return any values at all. Convert the
> internal libxl errors (ERROR_FAIL, ..., etc) to -1.

ITYM +1.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

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

[Ian Campbell]

On Mon, 2015-03-23 at 14:21 -0400, Konrad Rzeszutek Wilk wrote:
> 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>
> 
> ---
> [v2: Remove crud code as spotted by Boris]
> [v3: Moved crud code removal to another patch]
> ---
>  tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index b121d75..d7bd5d5 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5238,9 +5238,18 @@ 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 " \
> +        libxl_dominfo dominfo;
> +
> +        rc = libxl_domain_info(ctx, &dominfo, domid);
> +        if (rc)
> +        {
> +             if (rc == ERROR_DOMAIN_NOTFOUND)
> +                fprintf(stderr, "Domain %u does not exist.\n", domid);
> +            return 1;

Indentation is wrong on the if.

More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
are going to get rather tiresome, especially if/when there are other
interesting error codes. Perhaps we could arrange for something further
down the stack on libxl to log this sort of thing, such that xl can rely
on it already having been mentioned?

e.g. libxl_domain_info could do it perhaps?

Or perhaps a helper (in xl?) which produces some common logging for
common errors which could be called?

Re: [PATCH v3 2/7] libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

[Konrad Rzeszutek Wilk]

On Tue, Mar 24, 2015 at 03:15:30PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> > So that the callers can distinguish between an error and
> > an domain not found. The exposed API calls that are effected
> > by this are: libxl_domain_[remus_start,suspend,unpause,cdrom_insert,
> > set_vcpuonline]
> > 
> > We add an helper function to deal with the two types of errors:
> > libxl_domain_type2err. However for libxl_[pci,dom].c we just add
> > the extra check for LIBXL_DOMAIN_TYPE_NOTFOUND for simplicity
> > reasons.
> > 
> > Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Did I?

You suggested that 'libxl_set_vcpuonline' deal with all of the
various issues instead of having them in the 'vcpusset'. This would
bolting in libxl_set_vcpuonline' the proper error code support - which
this patch does.

> 
> Anyway, I'm not terribly keen on the approach taken here, sorry, in
> particular extending libxl_domain_type with a subset of the libxl_error
> codes and the shenanigans which are needed to convert between them.

:-)
> 
> Off hand I can think of 3 possible alternative solutions:
> 
> Arrange that the -ve error values in libxl_domain_type directly
> correspond to an appropriate libxl_error code, allowing code to do
>     if (type < 0) return type;
>     if (type < 0) { rc = type ; goto out }
> type stuff.
> 
> Change the prototype of libxl__domain_type to always return a libxl rc
> and on success return the type via a pointer argument.
> 
> Change the return type of libxl_domain_type to an int and return either
> a LIBXL_DOMAIN_TYPE_FOO or an ERROR_*.
> 
> They all have downsides (I don't much like the implicit cast between
> domain type and error code, changing the prototype will probably be
> disruptive, changing the return type means gcc can't catch missing
> switch statements for us).
> 
> Perhaps you or my fellow maintainers have a better idea or a preference
> for one of the above?

Drop the patch altogether and just leave the imprecise errors
that libxl_set_vcpuonline can return?

It is OK if it just returns LIBXL_DOMAIN_TYPE_INVALID - as the message about
what went wrong is most important. With patches:
 "libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)"
and
 "libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap"

we print now the error messages when the user mistakenly:
 1) picks the wrong domain id
 2) picks the wrong cpu count (too many)

So this patch can be ignored.
> 
> Ian.
> 

Ian vs Ian, round 0 Was:Re: [PATCH v3 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)

[Konrad Rzeszutek Wilk]

On Tue, Mar 24, 2015 at 03:41:46PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:21 -0400, Konrad Rzeszutek Wilk wrote:
> > 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>
> > 
> > ---
> > [v2: Remove crud code as spotted by Boris]
> > [v3: Moved crud code removal to another patch]
> > ---
> >  tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index b121d75..d7bd5d5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5238,9 +5238,18 @@ 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 " \
> > +        libxl_dominfo dominfo;
> > +
> > +        rc = libxl_domain_info(ctx, &dominfo, domid);
> > +        if (rc)
> > +        {
> > +             if (rc == ERROR_DOMAIN_NOTFOUND)
> > +                fprintf(stderr, "Domain %u does not exist.\n", domid);
> > +            return 1;
> 
> Indentation is wrong on the if.

Right!
> 
> More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
> are going to get rather tiresome, especially if/when there are other
> interesting error codes. Perhaps we could arrange for something further
> down the stack on libxl to log this sort of thing, such that xl can rely
> on it already having been mentioned?
> 
> e.g. libxl_domain_info could do it perhaps?

That would be the easiest way. Should I drop

libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

and just add an LIBXL__LOG_ERRNO and keep on returning the old error value
(ERROR_INVAL)?

Ian J recommended to add this new ERROR_DOMAIN_NOTFOUND in libxl_domain_info
so I think I will let you two fight it out! (changing the title to catch
Ian J's attention)

> 
> Or perhaps a helper (in xl?) which produces some common logging for
> common errors which could be called?

Nah, I am all for simplicity and just need these errors to be printed out.

We could also just leave the 
libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain

and I can add another patch that just adds an nice LIBXL__LOG_ERRNO in
the libxl_domain_info?

> 

Re: Ian vs Ian, round 0 Was:Re: [PATCH v3 7/7] libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)

[Ian Campbell]

On Tue, 2015-03-24 at 11:56 -0400, Konrad Rzeszutek Wilk wrote:
> > More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
> > are going to get rather tiresome, especially if/when there are other
> > interesting error codes. Perhaps we could arrange for something further
> > down the stack on libxl to log this sort of thing, such that xl can rely
> > on it already having been mentioned?
> > 
> > e.g. libxl_domain_info could do it perhaps?
> 
> That would be the easiest way. Should I drop
> 
> libxl: Add ERROR_DOMAIN_NOTFOUND for libxl_domain_info when it cannot find the domain
> 
> and just add an LIBXL__LOG_ERRNO and keep on returning the old error value
> (ERROR_INVAL)?

No.

I was suggesting that it should log *and* return DOMAIN_NOTFOUND, not
that the return code wasn't a good thing.

> Ian J recommended to add this new ERROR_DOMAIN_NOTFOUND in libxl_domain_info
> so I think I will let you two fight it out! (changing the title to catch
> Ian J's attention)

I haven't contradicted this.

Ian,

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

[Konrad Rzeszutek Wilk]

On Tue, Mar 24, 2015 at 03:41:46PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:21 -0400, Konrad Rzeszutek Wilk wrote:
> > 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>
> > 
> > ---
> > [v2: Remove crud code as spotted by Boris]
> > [v3: Moved crud code removal to another patch]
> > ---
> >  tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index b121d75..d7bd5d5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5238,9 +5238,18 @@ 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 " \
> > +        libxl_dominfo dominfo;
> > +
> > +        rc = libxl_domain_info(ctx, &dominfo, domid);
> > +        if (rc)
> > +        {
> > +             if (rc == ERROR_DOMAIN_NOTFOUND)
> > +                fprintf(stderr, "Domain %u does not exist.\n", domid);
> > +            return 1;
> 
> Indentation is wrong on the if.
> 
> More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
> are going to get rather tiresome, especially if/when there are other
> interesting error codes. Perhaps we could arrange for something further
> down the stack on libxl to log this sort of thing, such that xl can rely
> on it already having been mentioned?
> 
> e.g. libxl_domain_info could do it perhaps?

Would this patch work?

>From 9a0a0e581b29d9aa893d80962053383c235e9ad9 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 25 Mar 2015 13:36:58 -0400
Subject: [PATCH v3 4/8] libxl/libxl_domain_info: Log if domain not found.

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>
---
 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 c37d057..181b5bd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -707,8 +707,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



And then this patch would become:

>From 37d530f04a266e8d707b811bc7583f9d7b6fb18d Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 2 Feb 2015 16:18:39 -0500
Subject: [PATCH] libxl/vcpu-set - allow to decrease vcpu count on
 overcommitted guests (v3)

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>

---
[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..c77c691 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(&info);
+        if (rc)
+            return 1;
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
-- 
2.1.0

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

[Konrad Rzeszutek Wilk]

On Tue, Mar 24, 2015 at 03:22:55PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:20 -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.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/libxl.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 4152ee4..d2b5ff3 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5449,6 +5449,20 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
> >      return 0;
> >  }
> >  
> > +static int libxl__check_max(libxl__gc *gc, libxl_dominfo *info,
> > +                            libxl_bitmap *cpumap)
> > +{
> > +    int maxcpus = libxl_bitmap_count_set(cpumap);
> > +
> > +    if (maxcpus > info->vcpu_max_id + 1)
> > +    {
> > +        LOGE(ERROR, "You have a max of %d vCPUs and you want %d vCPUs!",
> > +             info->vcpu_max_id + 1, maxcpus);
> 
> Please avoid personal pronouns in libxl logs (I don't have a max of any
> number of vcpus, the domain does). Here "setting %d vcpus, max vcpus is
> %d" perhaps.
> 
> Both libxl__set_vcpuonline_qmp and libxl__set_vcpuonline_xenstore start
> with the same code to get info. Please could you move that (and the
> associated dispose) into libxl_set_vcpuonline and check the limit once
> up front (no need for helper then) and pass the info as a pointer to the
> specific functions.

Would this be OK ?

>From a160e64842f36f96f5da610c2498cf1770665dcb Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 20 Mar 2015 16:29:16 -0400
Subject: [PATCH v3 3/8] libxl: In libxl_set_vcpuonline check for maximum
 number of VCPUs against the cpumap.

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 underlaying libxl_set_vcpuonline code
together to take advantage for the QMP and XenStore ways.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 75 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f957713..c37d057 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5447,28 +5447,34 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
     return 0;
 }
 
+static int libxl__check_max(libxl__gc *gc, libxl_dominfo *info,
+                            libxl_bitmap *cpumap)
+{
+    int 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);
+        return ERROR_FAIL;
+    }
+    return 0;
+}
+
 static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
-                                         libxl_bitmap *cpumap)
+                                         libxl_bitmap *cpumap,
+                                         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");
@@ -5478,25 +5484,15 @@ 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, 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.
@@ -5506,7 +5502,6 @@ 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;
 }
 
@@ -5514,21 +5509,39 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
 {
     GC_INIT(ctx);
     int rc;
-    switch (libxl__domain_type(gc, domid)) {
+    libxl_domain_type type;
+    libxl_dominfo info;
+
+    libxl_dominfo_init(&info);
+    type = libxl__domain_type(gc, domid);
+
+    if (type == LIBXL_DOMAIN_TYPE_HVM || type == LIBXL_DOMAIN_TYPE_PV) {
+
+        rc = libxl_domain_info(CTX, &info, domid);
+        if (rc < 0) {
+            LOGE(ERROR, "getting domain info list");
+            goto out;
+        }
+        rc = libxl__check_max(gc, &info, cpumap);
+        if (rc)
+            goto out;
+    }
+
+    switch (type) {
     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;
     case LIBXL_DOMAIN_TYPE_NOTFOUND:
         rc = ERROR_DOMAIN_NOTFOUND;
@@ -5536,6 +5549,8 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
     default:
         rc = ERROR_INVAL;
     }
+out:
+    libxl_dominfo_dispose(&info);
     GC_FREE;
     return rc;
 }
-- 
2.1.0

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

[Ian Campbell]

On Wed, 2015-03-25 at 14:42 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 24, 2015 at 03:41:46PM +0000, Ian Campbell wrote:
> > On Mon, 2015-03-23 at 14:21 -0400, Konrad Rzeszutek Wilk wrote:
> > > 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>
> > > 
> > > ---
> > > [v2: Remove crud code as spotted by Boris]
> > > [v3: Moved crud code removal to another patch]
> > > ---
> > >  tools/libxl/xl_cmdimpl.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index b121d75..d7bd5d5 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -5238,9 +5238,18 @@ 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 " \
> > > +        libxl_dominfo dominfo;
> > > +
> > > +        rc = libxl_domain_info(ctx, &dominfo, domid);
> > > +        if (rc)
> > > +        {
> > > +             if (rc == ERROR_DOMAIN_NOTFOUND)
> > > +                fprintf(stderr, "Domain %u does not exist.\n", domid);
> > > +            return 1;
> > 
> > Indentation is wrong on the if.
> > 
> > More generally all of these rc == ERROR_DOMAIN_NOTFOUND check and prints
> > are going to get rather tiresome, especially if/when there are other
> > interesting error codes. Perhaps we could arrange for something further
> > down the stack on libxl to log this sort of thing, such that xl can rely
> > on it already having been mentioned?
> > 
> > e.g. libxl_domain_info could do it perhaps?
> 
> Would this patch work?

Assuming we aren't expecting to lookup domains which don't exist very
often then I think this would be fine.

> 
> From 9a0a0e581b29d9aa893d80962053383c235e9ad9 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 25 Mar 2015 13:36:58 -0400
> Subject: [PATCH v3 4/8] libxl/libxl_domain_info: Log if domain not found.
> 
> 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>
> ---
>  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 c37d057..181b5bd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -707,8 +707,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
> 
> 
> 
> And then this patch would become:
> 
> From 37d530f04a266e8d707b811bc7583f9d7b6fb18d Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 2 Feb 2015 16:18:39 -0500
> Subject: [PATCH] libxl/vcpu-set - allow to decrease vcpu count on
>  overcommitted guests (v3)
> 
> 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>
> 
> ---
> [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..c77c691 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(&info);
> +        if (rc)
> +            return 1;
>      }
>      rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
>      if (rc) {

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

[Ian Campbell]

On Wed, 2015-03-25 at 14:44 -0400, Konrad Rzeszutek Wilk wrote:

> +    if (type == LIBXL_DOMAIN_TYPE_HVM || type == LIBXL_DOMAIN_TYPE_PV) {

I think just do the libxl_domain_info unconditionally and handle the
error if it doesn't work.

Otherwise looked good, but please try and const-ify the info parameter.

Ian.