[PATCH v2] Fix xl vcpu-set to decrease amount of vCPUS without warnining.

[PATCH v2] Fix xl vcpu-set to decrease amount of vCPUS without warnining.

[Konrad Rzeszutek Wilk]

Hey,

This patchset had been floating in the past. The aim for is to allow
the user to do 'xl vcpu-set <domain-id> X' where the X can be bigger
than the amount of physical CPUs the machine has. You can do this by
launching an guest with huge amount of virtual CPUs without issues.

>From (http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg00216.html):
"

Let me rehash what we had in set in stone way back in 4.4:
 - The guest config ('maxvcpus') is permitted to be greater than the pCPUs.
   Ditto for the initially allocated ('vcpus') amounts. It is also
   OK to be different - 'vcpus' < 'maxvcpus', etc.

 - If the 'vcpus' < pCPUs and we want to increase it above pCPUs we should
   error out and print out a warning telling them to use --ignore-host.
   Regardless of the dominfo.max_vcpu_id - so if the max_vpcu_id is
   greater than pCPU and 'vcpu' < pCPU, we should still warn the user
   when increasing.

 - If the 'vcpus' > pCPUs and we want to decrease to be below pCPUs then
   we should do that without the warning.
   (this is what the patchset is fixing).

"

However during the fixes an request was made to have libxl_domain_info
return a more proper return value when there are no domains found - hence:
 [PATCH v2 1/5] libxl: Add ERROR_NOTFOUND for libxl_domain_info when

is part of this patchset.


 tools/libxl/libxl.c         | 18 +++++++++------
 tools/libxl/libxl.h         |  4 +++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 55 +++++++++++++++++++++++++++++----------------
 4 files changed, 51 insertions(+), 27 deletions(-)

[PATCH v2 1/5] libxl: Add ERROR_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         | 18 +++++++++++-------
 tools/libxl/libxl.h         |  4 +++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  7 ++++---
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 94b4d59..691efaf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -454,6 +454,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
 
     /* update /vm/<uuid>/name */
     rc = libxl_domain_info(ctx, &info, domid);
+    if (rc == ERROR_NOTFOUND)
+        goto x_rc;
     if (rc)
         goto x_fail;
 
@@ -698,7 +700,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_NOTFOUND;
 
     if (info_r)
         xcinfo2xlinfo(ctx, &xcinfo, info_r);
@@ -1572,7 +1574,7 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     switch(rc) {
     case 0:
         break;
-    case ERROR_INVAL:
+    case ERROR_NOTFOUND:
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "non-existant domain %d", domid);
     default:
         goto out;
@@ -5415,11 +5417,12 @@ 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;
     }
@@ -5446,14 +5449,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 5eec092..5164371 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1088,7 +1088,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 existance.
+ * Returns ERROR_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 47af340..69a91cc 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [
     (-17, "DEVICE_EXISTS"),
     (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
     (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
+    (-20, "NOTFOUND"),
     ], value_namespace = "")
 
 libxl_domain_type = Enumeration("domain_type", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 04faf98..2d7145f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4532,7 +4532,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_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
@@ -5302,7 +5302,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_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
             return -rc;
@@ -7495,7 +7495,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
         goto out;
     }
     for (c = 0; c < 10; c++) {
-        if (libxl_domain_info(ctx, &info, 0)) {
+        ret = -libxl_domain_info(ctx, &info, 0);
+        if (ret) {
             fprintf(stderr, "error on getting info for Domain-0\n");
             goto out;
         }
-- 
2.1.0

Re: [PATCH v2 1/5] libxl: Add ERROR_NOTFOUND for libxl_domain_info when it cannot find the domain

[Ian Campbell]

On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> And use that for all of its callers in the tree.

I wonder -- is it better to have a generic ERROR_NOTFOUND for anything
which is not found, or to have a specific ERROR_DOMAIN_NOTFOUND
(likewise for other things).

Any opinions? I'm inclined towards the latter, e.g. imagine
disk_remove(domid, disk) -- ERROR_NOTFOUND doesn't tell you if the disk
or the domain is missing. (this example may or may not be hypothetical,
I didn't chekc).

> @@ -454,6 +454,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>  
>      /* update /vm/<uuid>/name */
>      rc = libxl_domain_info(ctx, &info, domid);
> +    if (rc == ERROR_NOTFOUND)
> +        goto x_rc;
>      if (rc)
>          goto x_fail;

Might as well us x_rc for all failures, there seem little point in
squashing whatever libxl_domain_info returned into ERROR_FAIL for the
other cases either.

> @@ -5415,11 +5417,12 @@ 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;
>      }

I think rc needs setting to ERROR_FAIL after this to retain the same
behaviour for the subsequent failures of e.g. libxl__xs_get_dompath
which doesn't otherwise set rc and previous relied on the ERROR_FAIL
from above.

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5eec092..5164371 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1088,7 +1088,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 existance.

Could you fix the spelling of "existence" while you touch this line
please ;-)

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 47af340..69a91cc 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [
>      (-17, "DEVICE_EXISTS"),
>      (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
>      (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
> +    (-20, "NOTFOUND"),
>      ], value_namespace = "")

Do we need a #define LIBXL_HAVE_ERROR_NOTFOUND to indicate this is
available, I think we probably do.

> @@ -7495,7 +7495,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
>          goto out;
>      }
>      for (c = 0; c < 10; c++) {
> -        if (libxl_domain_info(ctx, &info, 0)) {
> +        ret = -libxl_domain_info(ctx, &info, 0);
> +        if (ret) {

I think we don't need to do this bit. (The current inconsistent exit
codes form xl notwithstanding)

[PATCH v2 2/5] libxl: vcpuset: Return error values.

[Konrad Rzeszutek Wilk]

The function does not return any values at all. Convert the
internal libxl ones (ERROR_FAIL, ..., etc) to positive values
and for the other cases just return standard libxl values.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2d7145f..454a895 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5013,17 +5013,18 @@ 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;
     libxl_bitmap cpumap;
+    int rc;
 
     libxl_bitmap_init(&cpumap);
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
     if (nr_vcpus == endptr) {
         fprintf(stderr, "Error: Invalid argument.\n");
-        return;
+        return -ERROR_INVAL;
     }
 
     /*
@@ -5036,22 +5037,25 @@ 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 -ERROR_INVAL;
         }
         /* 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 -rc;
     }
     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)
+        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d, rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
+    return rc ? -rc : 0;
 }
 
 int main_vcpuset(int argc, char **argv)
@@ -5070,8 +5074,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

Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Ian Campbell]

On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> The function does not return any values at all. Convert the
> internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> and for the other cases just return standard libxl values.

It's not clear why you want to do this, in particular returning
-ERROR_INVAL and inverting libxl error codes seems like a very strange
thing to be doing.

I think you should either use ERROR_INVAL (not inverted) and propagate
libxl rc's directly or convert them into something which suits xl, i.e.
0 and 1.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2d7145f..454a895 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5013,17 +5013,18 @@ 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;
>      libxl_bitmap cpumap;
> +    int rc;
>  
>      libxl_bitmap_init(&cpumap);
>      max_vcpus = strtoul(nr_vcpus, &endptr, 10);
>      if (nr_vcpus == endptr) {
>          fprintf(stderr, "Error: Invalid argument.\n");
> -        return;
> +        return -ERROR_INVAL;
>      }
>  
>      /*
> @@ -5036,22 +5037,25 @@ 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 -ERROR_INVAL;
>          }
>          /* 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 -rc;
>      }
>      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)
> +        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d, rc: %d\n", domid, max_vcpus, rc);
>  
>      libxl_bitmap_dispose(&cpumap);
> +    return rc ? -rc : 0;
>  }
>  
>  int main_vcpuset(int argc, char **argv)
> @@ -5070,8 +5074,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)

Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Ian Campbell]

On Wed, 2015-03-18 at 13:06 +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> > The function does not return any values at all. Convert the
> > internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> > and for the other cases just return standard libxl values.
> 
> It's not clear why you want to do this, in particular returning
> -ERROR_INVAL and inverting libxl error codes seems like a very strange
> thing to be doing.

BTW I know the xl error handling is horribly confused, and there are
even a small number of instances of -ERROR_* already, but I think those
are wrong and we shouldn't introduce more.

> 
> I think you should either use ERROR_INVAL (not inverted) and propagate
> libxl rc's directly or convert them into something which suits xl, i.e.
> 0 and 1.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2d7145f..454a895 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5013,17 +5013,18 @@ 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;
> >      libxl_bitmap cpumap;
> > +    int rc;
> >  
> >      libxl_bitmap_init(&cpumap);
> >      max_vcpus = strtoul(nr_vcpus, &endptr, 10);
> >      if (nr_vcpus == endptr) {
> >          fprintf(stderr, "Error: Invalid argument.\n");
> > -        return;
> > +        return -ERROR_INVAL;
> >      }
> >  
> >      /*
> > @@ -5036,22 +5037,25 @@ 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 -ERROR_INVAL;
> >          }
> >          /* 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 -rc;
> >      }
> >      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)
> > +        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d, rc: %d\n", domid, max_vcpus, rc);
> >  
> >      libxl_bitmap_dispose(&cpumap);
> > +    return rc ? -rc : 0;
> >  }
> >  
> >  int main_vcpuset(int argc, char **argv)
> > @@ -5070,8 +5074,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)
> 

Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1266 bytes --]

On Wed, 2015-03-18 at 13:08 +0000, Ian Campbell wrote:
> On Wed, 2015-03-18 at 13:06 +0000, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> > > The function does not return any values at all. Convert the
> > > internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> > > and for the other cases just return standard libxl values.
> > 
> > It's not clear why you want to do this, in particular returning
> > -ERROR_INVAL and inverting libxl error codes seems like a very strange
> > thing to be doing.
> 
> BTW I know the xl error handling is horribly confused, and there are
> even a small number of instances of -ERROR_* already, but I think those
> are wrong and we shouldn't introduce more.
> 
Indeed. I did some xl error code refactoring for a series of mine a few
days back, and as far as I could see, the most common pattern in xl is
returning 0 or 1.

FWIW, I think we should not diverge any further from that and, at some
point, convert 0/1 to EXIT_SUCCESS/EXIT_FAILURE.

> > I think you should either use ERROR_INVAL (not inverted) and propagate
> > libxl rc's directly or convert them into something which suits xl, i.e.
> > 0 and 1.
> > 
Again, +1 for 0 or 1.

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 02:09:09PM +0000, Dario Faggioli wrote:
> On Wed, 2015-03-18 at 13:08 +0000, Ian Campbell wrote:
> > On Wed, 2015-03-18 at 13:06 +0000, Ian Campbell wrote:
> > > On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> > > > The function does not return any values at all. Convert the
> > > > internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> > > > and for the other cases just return standard libxl values.
> > > 
> > > It's not clear why you want to do this, in particular returning
> > > -ERROR_INVAL and inverting libxl error codes seems like a very strange
> > > thing to be doing.
> > 
> > BTW I know the xl error handling is horribly confused, and there are
> > even a small number of instances of -ERROR_* already, but I think those
> > are wrong and we shouldn't introduce more.
> > 
> Indeed. I did some xl error code refactoring for a series of mine a few
> days back, and as far as I could see, the most common pattern in xl is
> returning 0 or 1.

Gah, I seem to have looked at the wrong examples and thought that was
the proper way!
> 
> FWIW, I think we should not diverge any further from that and, at some
> point, convert 0/1 to EXIT_SUCCESS/EXIT_FAILURE.
> 
> > > I think you should either use ERROR_INVAL (not inverted) and propagate
> > > libxl rc's directly or convert them into something which suits xl, i.e.
> > > 0 and 1.
> > > 
> Again, +1 for 0 or 1.
> 
> Regards,
> Dario

Re: [PATCH v2 2/5] libxl: vcpuset: Return error values.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 01:06:18PM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> > The function does not return any values at all. Convert the
> > internal libxl ones (ERROR_FAIL, ..., etc) to positive values
> > and for the other cases just return standard libxl values.
> 
> It's not clear why you want to do this, in particular returning
> -ERROR_INVAL and inverting libxl error codes seems like a very strange
> thing to be doing.

The ERROR_INVAL are negative values. Inverting them makes them
positive - which is what the rest of xl does for error vales?
> 
> I think you should either use ERROR_INVAL (not inverted) and propagate
> libxl rc's directly or convert them into something which suits xl, i.e.
> 0 and 1.

Oh, there is a lot of other code (xl <>) which return -ERROR_XYZ so that
the error values are positive.

How should 'xl' return errors? Just as 1 for failure or actually
use an -ERROR_XYZ so that folks can map them to -ERROR_XYZ?

> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2d7145f..454a895 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -5013,17 +5013,18 @@ 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;
> >      libxl_bitmap cpumap;
> > +    int rc;
> >  
> >      libxl_bitmap_init(&cpumap);
> >      max_vcpus = strtoul(nr_vcpus, &endptr, 10);
> >      if (nr_vcpus == endptr) {
> >          fprintf(stderr, "Error: Invalid argument.\n");
> > -        return;
> > +        return -ERROR_INVAL;
> >      }
> >  
> >      /*
> > @@ -5036,22 +5037,25 @@ 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 -ERROR_INVAL;
> >          }
> >          /* 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 -rc;
> >      }
> >      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)
> > +        fprintf(stderr, "libxl_set_vcpuonline failed domid=%d max_vcpus=%d, rc: %d\n", domid, max_vcpus, rc);
> >  
> >      libxl_bitmap_dispose(&cpumap);
> > +    return rc ? -rc : 0;
> >  }
> >  
> >  int main_vcpuset(int argc, char **argv)
> > @@ -5070,8 +5074,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)
> 
> 

[PATCH v2 3/5] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Konrad Rzeszutek Wilk]

The maximum number of VCPUs the guest can have is determined during
domain creation and is set by 'maxvcpus' parameter (in the guest
config). Trying to set the amount of vCPUs above said value
in vcpuset will result in an error - and we can catch it here
(instead of later in the function) and print a nice warning to the user.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Check ERROR_NOTFOUND return value]
---
 tools/libxl/xl_cmdimpl.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 454a895..ba0fd71 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5018,6 +5018,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     char *endptr;
     unsigned int max_vcpus, i;
     libxl_bitmap cpumap;
+    libxl_dominfo dominfo;
     int rc;
 
     libxl_bitmap_init(&cpumap);
@@ -5026,7 +5027,20 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
         fprintf(stderr, "Error: Invalid argument.\n");
         return -ERROR_INVAL;
     }
-
+    rc = libxl_domain_info(ctx, &dominfo, domid);
+    if (rc == ERROR_NOTFOUND) {
+        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
+        return -rc;
+    }
+    if (rc) {
+        fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
+        return -rc;
+    }
+    if (max_vcpus > dominfo.vcpu_max_id + 1) {
+        fprintf(stderr, "You have a max of %d vCPUs and you want %d vCPUs!\n",
+                dominfo.vcpu_max_id + 1, max_vcpus);
+        return -ERROR_INVAL;
+    }
     /*
      * Maximum amount of vCPUS the guest is allowed to set is limited
      * by the host's amount of pCPUs.
-- 
2.1.0

Re: [PATCH v2 3/5] libxl: vcpuset: Check max_vcpus argument against the maximum number of vCPUs the guest has set.

[Ian Campbell]

On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> The maximum number of VCPUs the guest can have is determined during
> domain creation and is set by 'maxvcpus' parameter (in the guest
> config). Trying to set the amount of vCPUs above said value
> in vcpuset will result in an error - and we can catch it here
> (instead of later in the function) and print a nice warning to the user.

I suppose the "later in the function" is in the return value of
libxl_set_vcpuonline?

I think this check would be better off done inside the library, i.e. in
libxl_set_vcpuonline, with a (probably new) suitable error code. The
libxl can log and xl can spot the situation and log something specific
(if indeed it needs to once libxl does).

Ian.

> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: Check ERROR_NOTFOUND return value]
> ---
>  tools/libxl/xl_cmdimpl.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 454a895..ba0fd71 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5018,6 +5018,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
>      char *endptr;
>      unsigned int max_vcpus, i;
>      libxl_bitmap cpumap;
> +    libxl_dominfo dominfo;
>      int rc;
>  
>      libxl_bitmap_init(&cpumap);
> @@ -5026,7 +5027,20 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
>          fprintf(stderr, "Error: Invalid argument.\n");
>          return -ERROR_INVAL;
>      }
> -
> +    rc = libxl_domain_info(ctx, &dominfo, domid);
> +    if (rc == ERROR_NOTFOUND) {
> +        fprintf(stderr, "Error: Domain %u does not exist.\n", domid);
> +        return -rc;
> +    }
> +    if (rc) {
> +        fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
> +        return -rc;
> +    }
> +    if (max_vcpus > dominfo.vcpu_max_id + 1) {
> +        fprintf(stderr, "You have a max of %d vCPUs and you want %d vCPUs!\n",
> +                dominfo.vcpu_max_id + 1, max_vcpus);
> +        return -ERROR_INVAL;
> +    }
>      /*
>       * Maximum amount of vCPUS the guest is allowed to set is limited
>       * by the host's amount of pCPUs.

[PATCH v2 4/5] 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>
---
 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 ba0fd71..981068d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5053,8 +5053,6 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                     " continue\n", host_cpu, max_vcpus);
             return -ERROR_INVAL;
         }
-        /* 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

Re: [PATCH v2 4/5] libxl: vcpuset: Remove useless limit on max_vcpus.

[Ian Campbell]

On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

[PATCH v2 5/5] 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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 981068d..c1093be 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5047,9 +5047,10 @@ 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 " \
+
+        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 -ERROR_INVAL;
         }
-- 
2.1.0

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

[Ian Campbell]

On Fri, 2015-03-13 at 16:26 -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>

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