[PATCH 0/9] Some (not only) cpupool related fixes and improvements

[PATCH 0/9] Some (not only) cpupool related fixes and improvements

[Dario Faggioli]

Hi everyone,

The main goal of this series is making it possible to specify ranges of pCPUs
when manipulating (creating, adding/removing pCPUs) cpupools. Something like
this:

 xl cpupool-cpu-remove Pool-node0 6-10

while, right now, only single pCPU IDs can be specified, which means that, to
achieve the above, we need to do this:

 xl cpupool-cpu-remove Pool-node0 6
 xl cpupool-cpu-remove Pool-node0 7
 xl cpupool-cpu-remove Pool-node0 8
 xl cpupool-cpu-remove Pool-node0 9
 xl cpupool-cpu-remove Pool-node0 10

This is done in patches 5/9, 6/9 and 7/9.

The series also add a new parameter to `xl list', '-c' or '--cpupool', which
shows in which cpupool the domain(s) lives. Such information was not easy to
find in any other way. One can use `xl sched-<schedule_of_cpupool_x>' and have,
as a side effect, something similar, but that's rather impractical.

Sample output for the new `xl list' can be found here:

 http://pastebin.com/qSagPimL

This happens in patch 3/9 and 4/9.

While there, the series includes some other, somewhat related, fixes of various
kind.

This is available as a git branch here:

 git://xenbits.xen.org/people/dariof/xen.git rel/cpupools/allow-ranges-v1
 http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/cpupools/allow-ranges-v1


Thanks and Regards,
Dario

---
Dario Faggioli (9):
  1/9  docs: RTDS is a valid alternative as a scheduler for a cpupool
  2/9  docs: fix `xl list' manpage entry
  3/9  xl: turn some int local variable into bool
  4/9  xl: add -c/--cpupool option to `xl list'
  5/9  libxl: introduce libxl_cpupool_cpu{add,remove}_cpumap()
  6/9  xl: enable using ranges of pCPUs when manipulating cpupools
  7/9  xl: enable using ranges of pCPUs when creating cpupools
  8/9  xl: make error reporting of cpupool subcommands consistent
  9/9  xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

 docs/man/xl.pod.1            |   33 +++++-
 docs/man/xlcpupool.cfg.pod.5 |   26 ++++-
 tools/examples/cpupool       |    2 
 tools/libxl/libxl.c          |   36 ++++++-
 tools/libxl/libxl.h          |    4 +
 tools/libxl/xl_cmdimpl.c     |  225 ++++++++++++++++++++++--------------------
 tools/libxl/xl_cmdtable.c    |    1 
 7 files changed, 209 insertions(+), 118 deletions(-)
--

[PATCH 1/9] docs: RTDS is a valid alternative as a scheduler for a cpupool

[Dario Faggioli]

so update the relevant manpage and the example file.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
Cc: Meng Xu <xumengpanda@gmail.com>
---
 docs/man/xlcpupool.cfg.pod.5 |    4 ++++
 tools/examples/cpupool       |    2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
index e32ce17..bb15cbe 100644
--- a/docs/man/xlcpupool.cfg.pod.5
+++ b/docs/man/xlcpupool.cfg.pod.5
@@ -74,6 +74,10 @@ the credit scheduler
 
 the credit2 scheduler
 
+=item B<rtds>
+
+the RTDS scheduler
+
 =item B<sedf>
 
 the SEDF scheduler
diff --git a/tools/examples/cpupool b/tools/examples/cpupool
index 01e62c8..73368e6 100644
--- a/tools/examples/cpupool
+++ b/tools/examples/cpupool
@@ -9,7 +9,7 @@
 # the name of the new cpupool
 name = "Example-Cpupool"
 
-# the scheduler to use: valid are e.g. credit, sedf, credit2
+# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
 sched = "credit"
 
 # list of cpus to use

Re: [PATCH 1/9] docs: RTDS is a valid alternative as a scheduler for a cpupool

[Meng Xu]

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

2015-03-06 12:20 GMT-05:00 Dario Faggioli <dario.faggioli@citrix.com>:

> so update the relevant manpage and the example file.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> Cc: Meng Xu <xumengpanda@gmail.com>
> ---
>  docs/man/xlcpupool.cfg.pod.5 |    4 ++++
>  tools/examples/cpupool       |    2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
> index e32ce17..bb15cbe 100644
> --- a/docs/man/xlcpupool.cfg.pod.5
> +++ b/docs/man/xlcpupool.cfg.pod.5
> @@ -74,6 +74,10 @@ the credit scheduler
>
>  the credit2 scheduler
>
> +=item B<rtds>
> +
> +the RTDS scheduler
> +
>  =item B<sedf>
>
>  the SEDF scheduler
> diff --git a/tools/examples/cpupool b/tools/examples/cpupool
> index 01e62c8..73368e6 100644
> --- a/tools/examples/cpupool
> +++ b/tools/examples/cpupool
> @@ -9,7 +9,7 @@
>  # the name of the new cpupool
>  name = "Example-Cpupool"
>
> -# the scheduler to use: valid are e.g. credit, sedf, credit2
> +# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
>  sched = "credit"
>
>  # list of cpus to use
>
>
​Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>​

Thank you very much!

Best,

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

[-- Attachment #1.2: Type: text/html, Size: 3137 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 1/9] docs: RTDS is a valid alternative as a scheduler for a cpupool

[Wei Liu]

On Fri, Mar 06, 2015 at 06:20:57PM +0100, Dario Faggioli wrote:
> so update the relevant manpage and the example file.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> Cc: Meng Xu <xumengpanda@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  docs/man/xlcpupool.cfg.pod.5 |    4 ++++
>  tools/examples/cpupool       |    2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
> index e32ce17..bb15cbe 100644
> --- a/docs/man/xlcpupool.cfg.pod.5
> +++ b/docs/man/xlcpupool.cfg.pod.5
> @@ -74,6 +74,10 @@ the credit scheduler
>  
>  the credit2 scheduler
>  
> +=item B<rtds>
> +
> +the RTDS scheduler
> +
>  =item B<sedf>
>  
>  the SEDF scheduler
> diff --git a/tools/examples/cpupool b/tools/examples/cpupool
> index 01e62c8..73368e6 100644
> --- a/tools/examples/cpupool
> +++ b/tools/examples/cpupool
> @@ -9,7 +9,7 @@
>  # the name of the new cpupool
>  name = "Example-Cpupool"
>  
> -# the scheduler to use: valid are e.g. credit, sedf, credit2
> +# the scheduler to use: valid are e.g. credit, credit2, rtds and sedf
>  sched = "credit"
>  
>  # list of cpus to use

[PATCH 2/9] docs: fix `xl list' manpage entry

[Dario Faggioli]

as it was not covering the '-n' option, which is present
since d743a223 ("xl: add node-affinity to the output of
`xl list`").

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.pod.1 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index cd80ffc..861ae21 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -304,6 +304,10 @@ Also prints the security labels.
 
 Also prints the domain UUIDs, the shutdown reason and security labels.
 
+=item B<-n>, <--numa>
+
+Also prints the domain NUMA node affinity.
+
 =back
 
 B<EXAMPLE>

Re: [PATCH 2/9] docs: fix `xl list' manpage entry

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:07PM +0100, Dario Faggioli wrote:
> as it was not covering the '-n' option, which is present
> since d743a223 ("xl: add node-affinity to the output of
> `xl list`").
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  docs/man/xl.pod.1 |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index cd80ffc..861ae21 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -304,6 +304,10 @@ Also prints the security labels.
>  
>  Also prints the domain UUIDs, the shutdown reason and security labels.
>  
> +=item B<-n>, <--numa>
> +
> +Also prints the domain NUMA node affinity.
> +
>  =back
>  
>  B<EXAMPLE>

[PATCH 3/9] xl: turn some int local variable into bool

[Dario Faggioli]

more specifically, the ones used as argument presence
flags in `xl list'.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..69469ff 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3520,7 +3520,7 @@ static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
     }
 }
 
-static void list_domains(int verbose, int context, int claim, int numa,
+static void list_domains(bool verbose, bool context, bool claim, bool numa,
                          const libxl_dominfo *info, int nb_domain)
 {
     int i;
@@ -4504,10 +4504,11 @@ int main_reboot(int argc, char **argv)
 
 int main_list(int argc, char **argv)
 {
-    int opt, verbose = 0;
-    int context = 0;
-    int details = 0;
-    int numa = 0;
+    int opt;
+    bool verbose = false;
+    bool context = false;
+    bool details = false;
+    bool numa = false;
     static struct option opts[] = {
         {"long", 0, 0, 'l'},
         {"verbose", 0, 0, 'v'},
@@ -4523,16 +4524,16 @@ int main_list(int argc, char **argv)
 
     SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
     case 'l':
-        details = 1;
+        details = true;
         break;
     case 'v':
-        verbose = 1;
+        verbose = true;
         break;
     case 'Z':
-        context = 1;
+        context = true;
         break;
     case 'n':
-        numa = 1;
+        numa = true;
         break;
     }

Re: [PATCH 3/9] xl: turn some int local variable into bool

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:15PM +0100, Dario Faggioli wrote:
> more specifically, the ones used as argument presence
> flags in `xl list'.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 4/9] xl: add -c/--cpupool option to `xl list'

[Dario Faggioli]

which, if provided, makes the command print a column
with the name of the cpupool of the listed domain(s).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 docs/man/xl.pod.1         |    4 ++++
 tools/libxl/xl_cmdimpl.c  |   20 ++++++++++++++++----
 tools/libxl/xl_cmdtable.c |    1 +
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 861ae21..c330016 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -304,6 +304,10 @@ Also prints the security labels.
 
 Also prints the domain UUIDs, the shutdown reason and security labels.
 
+=item B<-c>, <--cpupool>
+
+Also prints the cpupool the domain belong to.
+
 =item B<-n>, <--numa>
 
 Also prints the domain NUMA node affinity.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 69469ff..42b3954 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3521,7 +3521,7 @@ static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
 }
 
 static void list_domains(bool verbose, bool context, bool claim, bool numa,
-                         const libxl_dominfo *info, int nb_domain)
+                         bool cpupool, const libxl_dominfo *info, int nb_domain)
 {
     int i;
     static const char shutdown_reason_letters[]= "-rscw";
@@ -3535,6 +3535,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
     if (verbose) printf("   UUID                            Reason-Code\tSecurity Label");
     if (context && !verbose) printf("   Security Label");
     if (claim) printf("  Claimed");
+    if (cpupool) printf("         Cpupool");
     if (numa) {
         if (libxl_node_bitmap_alloc(ctx, &nodemap, 0)) {
             fprintf(stderr, "libxl_node_bitmap_alloc_failed.\n");
@@ -3579,6 +3580,11 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
             printf(" %5lu", (unsigned long)info[i].outstanding_memkb / 1024);
         if (verbose || context)
             printf(" %16s", info[i].ssid_label ? : "-");
+        if (cpupool) {
+            char *poolname = libxl_cpupoolid_to_name(ctx, info[i].cpupool);
+            printf("%16s", poolname);
+            free(poolname);
+        }
         if (numa) {
             libxl_domain_get_nodeaffinity(ctx, info[i].domid, &nodemap);
 
@@ -4508,11 +4514,13 @@ int main_list(int argc, char **argv)
     bool verbose = false;
     bool context = false;
     bool details = false;
+    bool cpupool = false;
     bool numa = false;
     static struct option opts[] = {
         {"long", 0, 0, 'l'},
         {"verbose", 0, 0, 'v'},
         {"context", 0, 0, 'Z'},
+        {"cpupool", 0, 0, 'c'},
         {"numa", 0, 0, 'n'},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
@@ -4522,7 +4530,7 @@ int main_list(int argc, char **argv)
     libxl_dominfo *info, *info_free=0;
     int nb_domain, rc;
 
-    SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
+    SWITCH_FOREACH_OPT(opt, "lvhZcn", opts, "list", 0) {
     case 'l':
         details = true;
         break;
@@ -4532,6 +4540,9 @@ int main_list(int argc, char **argv)
     case 'Z':
         context = true;
         break;
+    case 'c':
+        cpupool = true;
+        break;
     case 'n':
         numa = true;
         break;
@@ -4566,7 +4577,8 @@ int main_list(int argc, char **argv)
     if (details)
         list_domains_details(info, nb_domain);
     else
-        list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
+        list_domains(verbose, context, false /* claim */, numa, cpupool,
+                     info, nb_domain);
 
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);
@@ -6617,7 +6629,7 @@ int main_claims(int argc, char **argv)
     }
 
     list_domains(0 /* verbose */, 0 /* context */, 1 /* claim */,
-                 0 /* numa */, info, nb_domain);
+                 0 /* numa */, 0 /* cpupool */, info, nb_domain);
 
     libxl_dominfo_list_free(info, nb_domain);
     return 0;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 22ab63b..9284887 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -53,6 +53,7 @@ struct cmd_spec cmd_table[] = {
       "-l, --long              Output all VM details\n"
       "-v, --verbose           Prints out UUIDs and security context\n"
       "-Z, --context           Prints out security context\n"
+      "-c, --cpupool           Prints the cpupool the domain is in\n"
       "-n, --numa              Prints out NUMA node affinity"
     },
     { "destroy",

Re: [PATCH 4/9] xl: add -c/--cpupool option to `xl list'

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:23PM +0100, Dario Faggioli wrote:
> which, if provided, makes the command print a column
> with the name of the cpupool of the listed domain(s).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Only one nit. See below.

> -        list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
> +        list_domains(verbose, context, false /* claim */, numa, cpupool,
> +                     info, nb_domain);
>  
>      if (info_free)
>          libxl_dominfo_list_free(info, nb_domain);
> @@ -6617,7 +6629,7 @@ int main_claims(int argc, char **argv)
>      }
>  
>      list_domains(0 /* verbose */, 0 /* context */, 1 /* claim */,
> -                 0 /* numa */, info, nb_domain);
> +                 0 /* numa */, 0 /* cpupool */, info, nb_domain);

In previous patch, you changed all types to bool, but you forgot to use
"true" / "false" here.

Wei.

>  
>      libxl_dominfo_list_free(info, nb_domain);
>      return 0;
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 22ab63b..9284887 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -53,6 +53,7 @@ struct cmd_spec cmd_table[] = {
>        "-l, --long              Output all VM details\n"
>        "-v, --verbose           Prints out UUIDs and security context\n"
>        "-Z, --context           Prints out security context\n"
> +      "-c, --cpupool           Prints the cpupool the domain is in\n"
>        "-n, --numa              Prints out NUMA node affinity"
>      },
>      { "destroy",
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Dario Faggioli]

To add (removes) to (from) a cpupool all the pCPUs corresponding
to the bits that are set in the passed bitmap.

This is convenient and useful in order to implement, in xl,
the possibility of specifying ranges of pCPUs to be added
(removed) to (from) a cpupool, in the relevant commands.

While there, convert libxl_cpupool_cpu{add,remove} to use the
appropriate logging macro (LOGE()).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 tools/libxl/libxl.c |   36 ++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.h |    4 ++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 088786e..e06fe32 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6343,17 +6343,31 @@ out:
 
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;
 
     rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error moving cpu to cpupool");
+        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;
     return 0;
 }
 
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
+
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int rc = 0;
@@ -6388,17 +6402,31 @@ out:
 
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu)
 {
+    GC_INIT(ctx);
     int rc;
 
     rc = xc_cpupool_removecpu(ctx->xch, poolid, cpu);
     if (rc) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
-            "Error removing cpu from cpupool");
+        LOGE(ERROR, "Error removing cpu %d from cpupool", cpu);
+        GC_FREE;
         return ERROR_FAIL;
     }
+    GC_FREE;
     return 0;
 }
 
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap)
+{
+    int c, ncpus = 0;
+
+    libxl_for_each_set_bit(c, *cpumap) {
+        if (!libxl_cpupool_cpuremove(ctx, poolid, c))
+            ncpus++;
+    }
+    return ncpus;
+}
+
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
 {
     int ret = 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..7661999 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1456,8 +1456,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
 int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
 int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
+int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                const libxl_bitmap *cpumap);
 int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
+int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
+                                   const libxl_bitmap *cpumap);
 int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
 int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:32PM +0100, Dario Faggioli wrote:
> To add (removes) to (from) a cpupool all the pCPUs corresponding
> to the bits that are set in the passed bitmap.
> 
> This is convenient and useful in order to implement, in xl,
> the possibility of specifying ranges of pCPUs to be added
> (removed) to (from) a cpupool, in the relevant commands.
> 
> While there, convert libxl_cpupool_cpu{add,remove} to use the
> appropriate logging macro (LOGE()).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> ---
>  tools/libxl/libxl.c |   36 ++++++++++++++++++++++++++++++++----
>  tools/libxl/libxl.h |    4 ++++
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 088786e..e06fe32 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -6343,17 +6343,31 @@ out:
>  
>  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
>  {
> +    GC_INIT(ctx);
>      int rc;
>  
>      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
>      if (rc) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> -            "Error moving cpu to cpupool");
> +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> +        GC_FREE;
>          return ERROR_FAIL;
>      }
> +    GC_FREE;

Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

>      return 0;
>  }
>  
> +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> +                                const libxl_bitmap *cpumap)
> +{
> +    int c, ncpus = 0;
> +
> +    libxl_for_each_set_bit(c, *cpumap) {
> +        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
> +            ncpus++;
> +    }
> +    return ncpus;
> +}

I think returning a libxl error code on error and 0 on success is
better. At least this stay in line with libxl_cpupool_cpuadd_node.

> +
>  int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
>  {
>      int rc = 0;
> @@ -6388,17 +6402,31 @@ out:
>  
>  int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu)
>  {
> +    GC_INIT(ctx);
>      int rc;
>  
>      rc = xc_cpupool_removecpu(ctx->xch, poolid, cpu);
>      if (rc) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> -            "Error removing cpu from cpupool");
> +        LOGE(ERROR, "Error removing cpu %d from cpupool", cpu);
> +        GC_FREE;
>          return ERROR_FAIL;
>      }
> +    GC_FREE;
>      return 0;
>  }
>  
> +int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
> +                                   const libxl_bitmap *cpumap)
> +{
> +    int c, ncpus = 0;
> +
> +    libxl_for_each_set_bit(c, *cpumap) {
> +        if (!libxl_cpupool_cpuremove(ctx, poolid, c))
> +            ncpus++;
> +    }
> +    return ncpus;
> +}
> +
>  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus)
>  {
>      int ret = 0;
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..7661999 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1456,8 +1456,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
>  int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
>  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
>  int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> +                                const libxl_bitmap *cpumap);
>  int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
>  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> +int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
> +                                   const libxl_bitmap *cpumap);
>  int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
>  int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
>  

Missing #define LIBXl_HAVE_$FOO.

Wei.

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Dario Faggioli]

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

On Mon, 2015-03-09 at 10:39 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:32PM +0100, Dario Faggioli wrote:

> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -6343,17 +6343,31 @@ out:
> >  
> >  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
> >  {
> > +    GC_INIT(ctx);
> >      int rc;
> >  
> >      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
> >      if (rc) {
> > -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> > -            "Error moving cpu to cpupool");
> > +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> > +        GC_FREE;
> >          return ERROR_FAIL;
> >      }
> > +    GC_FREE;
> 
> Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.
> 
Ok.

> >      return 0;
> >  }
> >  
> > +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                const libxl_bitmap *cpumap)
> > +{
> > +    int c, ncpus = 0;
> > +
> > +    libxl_for_each_set_bit(c, *cpumap) {
> > +        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
> > +            ncpus++;
> > +    }
> > +    return ncpus;
> > +}
> 
> I think returning a libxl error code on error and 0 on success is
> better. At least this stay in line with libxl_cpupool_cpuadd_node.
> 
Will do.

> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -1456,8 +1456,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
> >  int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
> >  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
> >  int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                const libxl_bitmap *cpumap);
> >  int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
> >  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > +int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                   const libxl_bitmap *cpumap);
> >  int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
> >  int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
> >  
> 
> Missing #define LIBXl_HAVE_$FOO.
> 
So, do we need to do this every time we add a new function, even if not
changing any existing one, not adding fields to any data structure,
etc.?

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 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Wei Liu]

On Mon, Mar 09, 2015 at 11:31:09AM +0000, Dario Faggioli wrote:
[...]
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > > @@ -1456,8 +1456,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
> > >  int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
> > >  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
> > >  int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > > +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > > +                                const libxl_bitmap *cpumap);
> > >  int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
> > >  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > > +int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > > +                                   const libxl_bitmap *cpumap);
> > >  int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
> > >  int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
> > >  
> > 
> > Missing #define LIBXl_HAVE_$FOO.
> > 
> So, do we need to do this every time we add a new function, even if not
> changing any existing one, not adding fields to any data structure,
> etc.?
> 

IMHO this needs one.

Consider a new client which uses this API compiles against old library.
The same reason why we add LIBXL_HAVE_$FOO for any structure changes.

Wei.

> Regards,
> Dario

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Ian Campbell]

On Mon, 2015-03-09 at 10:39 +0000, Wei Liu wrote:
>      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
> >      if (rc) {
> > -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> > -            "Error moving cpu to cpupool");
> > +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> > +        GC_FREE;
> >          return ERROR_FAIL;
> >      }
> > +    GC_FREE;
> 
> Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.

TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is
otherwise unneeded in the entire call-chain, like here, is a reasonable
exception to the usual rule.

Ian.

Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Dario Faggioli]

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

On Wed, 2015-03-11 at 16:42 +0000, Ian Campbell wrote:
> On Mon, 2015-03-09 at 10:39 +0000, Wei Liu wrote:
> >      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
> > >      if (rc) {
> > > -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> > > -            "Error moving cpu to cpupool");
> > > +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> > > +        GC_FREE;
> > >          return ERROR_FAIL;
> > >      }
> > > +    GC_FREE;
> > 
> > Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.
> 
> TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is
> otherwise unneeded in the entire call-chain, like here, is a reasonable
> exception to the usual rule.
> 
And I distinctly remember someone of you tools people saying, about
another patch, that "using the new macro is well worth adding a
GC_INIT/FREE" :-D

Of course, I'm fine with leaving this alone... just let me know what you
prefer. :-)

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 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Ian Campbell]

On Wed, 2015-03-11 at 16:50 +0000, Dario Faggioli wrote:
> On Wed, 2015-03-11 at 16:42 +0000, Ian Campbell wrote:
> > On Mon, 2015-03-09 at 10:39 +0000, Wei Liu wrote:
> > >      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
> > > >      if (rc) {
> > > > -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> > > > -            "Error moving cpu to cpupool");
> > > > +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> > > > +        GC_FREE;
> > > >          return ERROR_FAIL;
> > > >      }
> > > > +    GC_FREE;
> > > 
> > > Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.
> > 
> > TBH I think using LIBXL__LOG_ERRNOVAL in functions where the GC is
> > otherwise unneeded in the entire call-chain, like here, is a reasonable
> > exception to the usual rule.
> > 
> And I distinctly remember someone of you tools people saying, about
> another patch, that "using the new macro is well worth adding a
> GC_INIT/FREE" :-D

Could have even be me on another day ;-)

> Of course, I'm fine with leaving this alone... just let me know what you
> prefer. :-)

Either use GC+goto style error paths or leave it alone, either is fine
by me.

Ian.

[PATCH 6/9] xl: enable using ranges of pCPUs when manipulating cpupools

[Dario Faggioli]

in fact, right now, xl sub-commands 'cpupool-cpu-add' and
'cpupool-cpu-remove' only accept the specification of one
pCPU to be added or removed to/from a cpupool.

With this change, they can deal with ranges, like "4-8",
or "node:1,12-18,^14". The syntax is exactly the same one
that is supported by the 'vcpu-pin' subcommand, and
specifying just one pCPU still works, of course.

This make things more flexible, more consistent, and also
improves error handling, as the pCPU range parsing routine
already present in xl is more reliable than just a call
to atoi().

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 docs/man/xl.pod.1        |   25 ++++++++++--
 tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 57 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index c330016..ef0d763 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1147,13 +1147,30 @@ This is possible only if no domain is active in the cpu-pool.
 
 Renames a cpu-pool to I<newname>.
 
-=item B<cpupool-cpu-add> I<cpu-pool> I<cpu-nr|node:node-nr>
+=item B<cpupool-cpu-add> I<cpu-pool> I<cpus|node:nodes>
 
-Adds a cpu or all cpus of a numa node to a cpu-pool.
+Adds I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes
+can be specified as single pCPU/node IDs or as ranges.
 
-=item B<cpupool-cpu-remove> I<cpu-nr|node:node-nr>
+For example:
+
+ (a) xl cpupool-cpu-add mypool 4
+ (b) xl cpupool-cpu-add mypool 1,5,10-16,^13
+ (c) xl cpupool-cpu-add mypool node:0,nodes:2-3,^10-12,8
+
+means adding pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
+and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,
+plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
+
+All the specified pCPUs that can be added to the cpupool will be added
+to it. If some pCPU can't (e.g., because they're already part of another
+cpupool), an error is reported about each one of them.
+
+=item B<cpupool-cpu-remove> I<cpus|node:nodes>
 
-Removes a cpu or all cpus of a numa node from a cpu-pool.
+Removes I<cpus> or full NUMA I<nodes> from I<cpu-pool>. pCPUs and NUMA
+nodes can be specified as single pCPU/node IDs or as ranges, using the
+exact same syntax as in B<cpupool-cpu-add> above.
 
 =item B<cpupool-migrate> I<domain> I<cpu-pool>
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 42b3954..c748ba0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -760,7 +760,7 @@ static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
  * single cpus or as eintire NUMA nodes) and turns it into the
  * corresponding libxl_bitmap (in cpumap).
  */
-static int vcpupin_parse(const char *cpu, libxl_bitmap *cpumap)
+static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)
 {
     char *ptr, *saveptr = NULL, *buf = strdup(cpu);
     int rc = 0;
@@ -870,7 +870,7 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
                 exit(1);
             }
 
-            if (vcpupin_parse(buf, &vcpu_affinity_array[j]))
+            if (cpurange_parse(buf, &vcpu_affinity_array[j]))
                 exit(1);
 
             j++;
@@ -887,7 +887,7 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
             exit(1);
         }
 
-        if (vcpupin_parse(buf, &vcpu_affinity_array[0]))
+        if (cpurange_parse(buf, &vcpu_affinity_array[0]))
             exit(1);
 
         for (i = 1; i < b_info->max_vcpus; i++) {
@@ -4979,7 +4979,7 @@ int main_vcpupin(int argc, char **argv)
      */
     if (!strcmp(hard_str, "-"))
         hard = NULL;
-    else if (vcpupin_parse(hard_str, hard))
+    else if (cpurange_parse(hard_str, hard))
         goto out;
     /*
      * Soft affinity is handled similarly. Only difference: we also want
@@ -4987,7 +4987,7 @@ int main_vcpupin(int argc, char **argv)
      */
     if (argc <= optind+3 || !strcmp(soft_str, "-"))
         soft = NULL;
-    else if (vcpupin_parse(soft_str, soft))
+    else if (cpurange_parse(soft_str, soft))
         goto out;
 
     if (dryrun_only) {
@@ -7325,44 +7325,38 @@ int main_cpupoolcpuadd(int argc, char **argv)
     int opt;
     const char *pool;
     uint32_t poolid;
-    int cpu;
-    int node;
-    int n;
+    libxl_bitmap cpumap;
+    int n, rc = 1;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-add", 2) {
         /* No options */
     }
 
-    pool = argv[optind++];
-    node = -1;
-    cpu = -1;
-    if (strncmp(argv[optind], "node:", 5) == 0) {
-        node = atoi(argv[optind] + 5);
-    } else {
-        cpu = atoi(argv[optind]);
+    libxl_bitmap_init(&cpumap);
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
+        fprintf(stderr, "Unable to allocate cpumap");
+        return 1;
     }
 
+    pool = argv[optind++];
+    if (cpurange_parse(argv[optind], &cpumap))
+        goto out;
+
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
-    }
-
-    if (cpu >= 0) {
-        return -libxl_cpupool_cpuadd(ctx, poolid, cpu);
+        goto out;
     }
 
-    if (libxl_cpupool_cpuadd_node(ctx, poolid, node, &n)) {
-        fprintf(stderr, "libxl_cpupool_cpuadd_node failed\n");
-        return -ERROR_FAIL;
-    }
+    n = libxl_cpupool_cpuadd_cpumap(ctx, poolid, &cpumap);
+    if (n != libxl_bitmap_count_set(&cpumap))
+        fprintf(stderr, "not all cpus have been added to the cpupool\n");
 
-    if (n > 0) {
-        return 0;
-    }
+    rc = 0;
 
-    fprintf(stderr, "no free cpu found\n");
-    return -ERROR_FAIL;
+out:
+    libxl_bitmap_dispose(&cpumap);
+    return rc;
 }
 
 int main_cpupoolcpuremove(int argc, char **argv)
@@ -7370,44 +7364,38 @@ int main_cpupoolcpuremove(int argc, char **argv)
     int opt;
     const char *pool;
     uint32_t poolid;
-    int cpu;
-    int node;
-    int n;
+    libxl_bitmap cpumap;
+    int n, rc = 1;
+
+    libxl_bitmap_init(&cpumap);
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
+        fprintf(stderr, "Unable to allocate cpumap");
+        return 1;
+    }
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-remove", 2) {
         /* No options */
     }
 
     pool = argv[optind++];
-    node = -1;
-    cpu = -1;
-    if (strncmp(argv[optind], "node:", 5) == 0) {
-        node = atoi(argv[optind] + 5);
-    } else {
-        cpu = atoi(argv[optind]);
-    }
+    if (cpurange_parse(argv[optind], &cpumap))
+        goto out;
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
-    }
-
-    if (cpu >= 0) {
-        return -libxl_cpupool_cpuremove(ctx, poolid, cpu);
+        goto out;
     }
 
-    if (libxl_cpupool_cpuremove_node(ctx, poolid, node, &n)) {
-        fprintf(stderr, "libxl_cpupool_cpuremove_node failed\n");
-        return -ERROR_FAIL;
-    }
+    n = libxl_cpupool_cpuremove_cpumap(ctx, poolid, &cpumap);
+    if (n != libxl_bitmap_count_set(&cpumap))
+        fprintf(stderr, "not all cpus could be removed from the cpupool\n");
 
-    if (n == 0) {
-        fprintf(stderr, "no cpu of node found in cpupool\n");
-        return -ERROR_FAIL;
-    }
+    rc = 0;
 
-    return 0;
+out:
+    libxl_bitmap_dispose(&cpumap);
+    return rc;
 }
 
 int main_cpupoolmigrate(int argc, char **argv)

Re: [PATCH 6/9] xl: enable using ranges of pCPUs when manipulating cpupools

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:42PM +0100, Dario Faggioli wrote:
> in fact, right now, xl sub-commands 'cpupool-cpu-add' and
> 'cpupool-cpu-remove' only accept the specification of one
> pCPU to be added or removed to/from a cpupool.
> 
> With this change, they can deal with ranges, like "4-8",
> or "node:1,12-18,^14". The syntax is exactly the same one
> that is supported by the 'vcpu-pin' subcommand, and
> specifying just one pCPU still works, of course.
> 
> This make things more flexible, more consistent, and also
> improves error handling, as the pCPU range parsing routine
> already present in xl is more reliable than just a call
> to atoi().
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> ---
>  docs/man/xl.pod.1        |   25 ++++++++++--
>  tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
>  2 files changed, 62 insertions(+), 57 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index c330016..ef0d763 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1147,13 +1147,30 @@ This is possible only if no domain is active in the cpu-pool.
>  
>  Renames a cpu-pool to I<newname>.
>  
> -=item B<cpupool-cpu-add> I<cpu-pool> I<cpu-nr|node:node-nr>
> +=item B<cpupool-cpu-add> I<cpu-pool> I<cpus|node:nodes>
>  
> -Adds a cpu or all cpus of a numa node to a cpu-pool.
> +Adds I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes
                   ^^^^
You can also specify some cpus inside a NUMA node.

> +can be specified as single pCPU/node IDs or as ranges.
>  
> -=item B<cpupool-cpu-remove> I<cpu-nr|node:node-nr>
> +For example:
> +
> + (a) xl cpupool-cpu-add mypool 4
> + (b) xl cpupool-cpu-add mypool 1,5,10-16,^13
> + (c) xl cpupool-cpu-add mypool node:0,nodes:2-3,^10-12,8
> +
> +means adding pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
> +and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,
> +plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
> +
> +All the specified pCPUs that can be added to the cpupool will be added
> +to it. If some pCPU can't (e.g., because they're already part of another
> +cpupool), an error is reported about each one of them.
> +
> +=item B<cpupool-cpu-remove> I<cpus|node:nodes>
>  
> -Removes a cpu or all cpus of a numa node from a cpu-pool.
> +Removes I<cpus> or full NUMA I<nodes> from I<cpu-pool>. pCPUs and NUMA

Ditto.

> +nodes can be specified as single pCPU/node IDs or as ranges, using the
> +exact same syntax as in B<cpupool-cpu-add> above.
>  
>  =item B<cpupool-migrate> I<domain> I<cpu-pool>
>  

The changes look good to me.

Wei.

Re: [PATCH 6/9] xl: enable using ranges of pCPUs when manipulating cpupools

[Dario Faggioli]

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

On Mon, 2015-03-09 at 10:51 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:42PM +0100, Dario Faggioli wrote:

> > ---
> >  docs/man/xl.pod.1        |   25 ++++++++++--
> >  tools/libxl/xl_cmdimpl.c |   94 ++++++++++++++++++++--------------------------
> >  2 files changed, 62 insertions(+), 57 deletions(-)
> > 
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > index c330016..ef0d763 100644
> > --- a/docs/man/xl.pod.1
> > +++ b/docs/man/xl.pod.1
> > @@ -1147,13 +1147,30 @@ This is possible only if no domain is active in the cpu-pool.
> >  
> >  Renames a cpu-pool to I<newname>.
> >  
> > -=item B<cpupool-cpu-add> I<cpu-pool> I<cpu-nr|node:node-nr>
> > +=item B<cpupool-cpu-add> I<cpu-pool> I<cpus|node:nodes>
> >  
> > -Adds a cpu or all cpus of a numa node to a cpu-pool.
> > +Adds I<cpus> or full NUMA I<nodes> to I<cpu-pool>. pCPUs and NUMA nodes
>                    ^^^^
> You can also specify some cpus inside a NUMA node.
> 
Well, if you use "node:1,2", that means _all_ the cpus of NUMA node 1
and 2. Then, of course, you can add only some of then by just saying
"0,1,8,9", or something like "node:1,^2-7", so yes, of course you can
specify only some of the cpus of a NUMA node, but that was meant at
describing what "node:" does.

That being said, I'll look for a better wording, or just use

"Adds pCPUs and/or NUMA nodes to I<cpu-pool>"

and leave it to the more detailed explanation and to the examples below.

> > +can be specified as single pCPU/node IDs or as ranges.
> >  
> > -=item B<cpupool-cpu-remove> I<cpu-nr|node:node-nr>
> > +For example:
> > +
> > + (a) xl cpupool-cpu-add mypool 4
> > + (b) xl cpupool-cpu-add mypool 1,5,10-16,^13
> > + (c) xl cpupool-cpu-add mypool node:0,nodes:2-3,^10-12,8
> > +
> > +means adding pCPU 4 to mypool, in (a); adding pCPUs 1,5,10,11,12,14,15
> > +and 16, in (b); and adding all the pCPUs of NUMA nodes 0, 2 and 3,
> > +plus pCPU 8, but keeping out pCPUs 10,11,12, in (c).
> > +
> > +All the specified pCPUs that can be added to the cpupool will be added
> > +to it. If some pCPU can't (e.g., because they're already part of another
> > +cpupool), an error is reported about each one of them.
> > +

Thanks and 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

[PATCH 7/9] xl: enable using ranges of pCPUs when creating cpupools

[Dario Faggioli]

instead of just list of single pCPUs or NUMA node IDs, as
it happens right now.

On the other hand, after this change, strings containing
pCPUs and NUMA node ranges is supported. The syntax is the
same one supported by the "cpus" and "cpus_soft" config
switch, i.e., "4-8" or "node:1,12-18,^14".

This make things more flexible, more consistent, and also
improves error handling, as the pCPU range parsing routine
already present in xl is more reliable than just a call
to atoi().

While there, remove a redundant error check in the legacy syntax
handling (libxl_bitmap_test() already checks the index being
within the size of the bitmap).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
 tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
index bb15cbe..2ff8ee8 100644
--- a/docs/man/xlcpupool.cfg.pod.5
+++ b/docs/man/xlcpupool.cfg.pod.5
@@ -93,10 +93,26 @@ Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or
 a list of integers) to be member of the cpupool. The free cpus in the
 specified nodes are allocated in the new cpupool.
 
-=item B<cpus="CPUS">
+=item B<cpus="CPU-LIST">
 
-The specified C<CPUS> are allocated in the new cpupool. All cpus must
-be free. Must not be specified together with B<nodes>.
+Specifies the cpus that will be member of the cpupool. All the specified
+cpus must be free, or creation will fail. C<CPU-LIST> may be specified
+as follows:
+
+=over 4
+
+=item ["2", "3", "5"]
+
+means that cpus 2,3,5 will be member of the cpupool.
+
+=item "0-3,5,^1"
+
+means that cpus 0,2,3 and 5 will be member of the cpupool. A "node:" or
+"nodes:" modifier can be used. E.g., "0,node:1,nodes:2-3,^10-13" means
+that pcpus 0, plus all the cpus of NUMA nodes 1,2,3 with the exception
+of cpus 10,11,12,13 will be memeber of the cpupool.
+
+=back
 
 If neither B<nodes> nor B<cpus> are specified only the first free cpu
 found will be allocated in the new cpupool.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c748ba0..e6d1234 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7163,18 +7163,29 @@ int main_cpupoolcreate(int argc, char **argv)
             fprintf(stderr, "no free cpu found\n");
             goto out_cfg;
         }
-    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
+    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 1)) {
         n_cpus = 0;
         while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
             i = atoi(buf);
-            if ((i < 0) || (i >= freemap.size * 8) ||
-                !libxl_bitmap_test(&freemap, i)) {
+            if ((i < 0) || !libxl_bitmap_test(&freemap, i)) {
                 fprintf(stderr, "cpu %d illegal or not free\n", i);
                 goto out_cfg;
             }
             libxl_bitmap_set(&cpumap, i);
             n_cpus++;
         }
+    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
+        if (cpurange_parse(buf, &cpumap))
+            goto out_cfg;
+
+        n_cpus = 0;
+        libxl_for_each_set_bit(i, cpumap) {
+            if (!libxl_bitmap_test(&freemap, i)) {
+                fprintf(stderr, "cpu %d illegal or not free\n", i);
+                goto out_cfg;
+            }
+            n_cpus++;
+        }
     } else
         n_cpus = 0;

Re: [PATCH 7/9] xl: enable using ranges of pCPUs when creating cpupools

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:51PM +0100, Dario Faggioli wrote:
> instead of just list of single pCPUs or NUMA node IDs, as
> it happens right now.
> 
> On the other hand, after this change, strings containing
> pCPUs and NUMA node ranges is supported. The syntax is the
> same one supported by the "cpus" and "cpus_soft" config
> switch, i.e., "4-8" or "node:1,12-18,^14".
> 
> This make things more flexible, more consistent, and also
> improves error handling, as the pCPU range parsing routine
> already present in xl is more reliable than just a call
> to atoi().
> 
> While there, remove a redundant error check in the legacy syntax
> handling (libxl_bitmap_test() already checks the index being
> within the size of the bitmap).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> ---
>  docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
>  tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
> index bb15cbe..2ff8ee8 100644
> --- a/docs/man/xlcpupool.cfg.pod.5
> +++ b/docs/man/xlcpupool.cfg.pod.5
> @@ -93,10 +93,26 @@ Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or
>  a list of integers) to be member of the cpupool. The free cpus in the
>  specified nodes are allocated in the new cpupool.
>  
> -=item B<cpus="CPUS">
> +=item B<cpus="CPU-LIST">
>  
> -The specified C<CPUS> are allocated in the new cpupool. All cpus must
> -be free. Must not be specified together with B<nodes>.
> +Specifies the cpus that will be member of the cpupool. All the specified
> +cpus must be free, or creation will fail. C<CPU-LIST> may be specified
> +as follows:
> +
> +=over 4
> +
> +=item ["2", "3", "5"]
> +
> +means that cpus 2,3,5 will be member of the cpupool.
> +

I suppose this is the old syntax?

I'm asking because I want to be sure we still support the old syntax.  I
think we still support the old syntax from the look of the changes below
but I'd better get confirmation from you.

> +=item "0-3,5,^1"
> +
> +means that cpus 0,2,3 and 5 will be member of the cpupool. A "node:" or
> +"nodes:" modifier can be used. E.g., "0,node:1,nodes:2-3,^10-13" means
> +that pcpus 0, plus all the cpus of NUMA nodes 1,2,3 with the exception
> +of cpus 10,11,12,13 will be memeber of the cpupool.
> +
> +=back
>  
>  If neither B<nodes> nor B<cpus> are specified only the first free cpu
>  found will be allocated in the new cpupool.
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c748ba0..e6d1234 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7163,18 +7163,29 @@ int main_cpupoolcreate(int argc, char **argv)
>              fprintf(stderr, "no free cpu found\n");
>              goto out_cfg;
>          }
> -    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
> +    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 1)) {
>          n_cpus = 0;
>          while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
>              i = atoi(buf);
> -            if ((i < 0) || (i >= freemap.size * 8) ||
> -                !libxl_bitmap_test(&freemap, i)) {
> +            if ((i < 0) || !libxl_bitmap_test(&freemap, i)) {
>                  fprintf(stderr, "cpu %d illegal or not free\n", i);
>                  goto out_cfg;
>              }
>              libxl_bitmap_set(&cpumap, i);
>              n_cpus++;
>          }
> +    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
> +        if (cpurange_parse(buf, &cpumap))

Perhaps print something to say the cpu range specified is invalid?

Wei.

> +            goto out_cfg;
> +
> +        n_cpus = 0;
> +        libxl_for_each_set_bit(i, cpumap) {
> +            if (!libxl_bitmap_test(&freemap, i)) {
> +                fprintf(stderr, "cpu %d illegal or not free\n", i);
> +                goto out_cfg;
> +            }
> +            n_cpus++;
> +        }
>      } else
>          n_cpus = 0;
>  

Re: [PATCH 7/9] xl: enable using ranges of pCPUs when creating cpupools

[Dario Faggioli]

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

On Mon, 2015-03-09 at 10:58 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:51PM +0100, Dario Faggioli wrote:

> > ---
> >  docs/man/xlcpupool.cfg.pod.5 |   22 +++++++++++++++++++---
> >  tools/libxl/xl_cmdimpl.c     |   17 ++++++++++++++---
> >  2 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
> > index bb15cbe..2ff8ee8 100644
> > --- a/docs/man/xlcpupool.cfg.pod.5
> > +++ b/docs/man/xlcpupool.cfg.pod.5
> > @@ -93,10 +93,26 @@ Specifies the cpus of the NUMA-nodes given in C<NODES> (an integer or
> >  a list of integers) to be member of the cpupool. The free cpus in the
> >  specified nodes are allocated in the new cpupool.
> >  
> > -=item B<cpus="CPUS">
> > +=item B<cpus="CPU-LIST">
> >  
> > -The specified C<CPUS> are allocated in the new cpupool. All cpus must
> > -be free. Must not be specified together with B<nodes>.
> > +Specifies the cpus that will be member of the cpupool. All the specified
> > +cpus must be free, or creation will fail. C<CPU-LIST> may be specified
> > +as follows:
> > +
> > +=over 4
> > +
> > +=item ["2", "3", "5"]
> > +
> > +means that cpus 2,3,5 will be member of the cpupool.
> > +
> 
> I suppose this is the old syntax?
> 
It is.

> I'm asking because I want to be sure we still support the old syntax.  I
> think we still support the old syntax from the look of the changes below
> but I'd better get confirmation from you.
> 
I also think we should, and, in fact, we do. :-)

> > +=item "0-3,5,^1"
> > +
> > +means that cpus 0,2,3 and 5 will be member of the cpupool. A "node:" or
> > +"nodes:" modifier can be used. E.g., "0,node:1,nodes:2-3,^10-13" means
> > +that pcpus 0, plus all the cpus of NUMA nodes 1,2,3 with the exception
> > +of cpus 10,11,12,13 will be memeber of the cpupool.
> > +
> > +=back
> >  
> >  If neither B<nodes> nor B<cpus> are specified only the first free cpu
> >  found will be allocated in the new cpupool.
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index c748ba0..e6d1234 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -7163,18 +7163,29 @@ int main_cpupoolcreate(int argc, char **argv)
> >              fprintf(stderr, "no free cpu found\n");
> >              goto out_cfg;
> >          }
> > -    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
> > +    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 1)) {
> >          n_cpus = 0;
> >          while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
> >              i = atoi(buf);
> > -            if ((i < 0) || (i >= freemap.size * 8) ||
> > -                !libxl_bitmap_test(&freemap, i)) {
> > +            if ((i < 0) || !libxl_bitmap_test(&freemap, i)) {
> >                  fprintf(stderr, "cpu %d illegal or not free\n", i);
> >                  goto out_cfg;
> >              }
> >              libxl_bitmap_set(&cpumap, i);
> >              n_cpus++;
> >          }
> > +    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
> > +        if (cpurange_parse(buf, &cpumap))
> 
> Perhaps print something to say the cpu range specified is invalid?
> 
It happens already inside cpurange_parse() (well, to be precise, it's
inside update_cpumap_range() ), which is in a better position for
printing the most meaningful error messages.

If you check any other call site of cpurange_parse(), none prints
anything on failure, exactly for that reason.

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 7/9] xl: enable using ranges of pCPUs when creating cpupools

[Wei Liu]

On Mon, Mar 09, 2015 at 11:18:57AM +0000, Dario Faggioli wrote:
[...]
> > > +
> > 
> > I suppose this is the old syntax?
> > 
> It is.
> 
> > I'm asking because I want to be sure we still support the old syntax.  I
> > think we still support the old syntax from the look of the changes below
> > but I'd better get confirmation from you.
> > 
> I also think we should, and, in fact, we do. :-)
> 

Thanks for confirming.

> > > +=item "0-3,5,^1"
> > > +
> > > +means that cpus 0,2,3 and 5 will be member of the cpupool. A "node:" or
> > > +"nodes:" modifier can be used. E.g., "0,node:1,nodes:2-3,^10-13" means
> > > +that pcpus 0, plus all the cpus of NUMA nodes 1,2,3 with the exception
> > > +of cpus 10,11,12,13 will be memeber of the cpupool.
> > > +
> > > +=back
> > >  
> > >  If neither B<nodes> nor B<cpus> are specified only the first free cpu
> > >  found will be allocated in the new cpupool.
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index c748ba0..e6d1234 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -7163,18 +7163,29 @@ int main_cpupoolcreate(int argc, char **argv)
> > >              fprintf(stderr, "no free cpu found\n");
> > >              goto out_cfg;
> > >          }
> > > -    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
> > > +    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 1)) {
> > >          n_cpus = 0;
> > >          while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
> > >              i = atoi(buf);
> > > -            if ((i < 0) || (i >= freemap.size * 8) ||
> > > -                !libxl_bitmap_test(&freemap, i)) {
> > > +            if ((i < 0) || !libxl_bitmap_test(&freemap, i)) {
> > >                  fprintf(stderr, "cpu %d illegal or not free\n", i);
> > >                  goto out_cfg;
> > >              }
> > >              libxl_bitmap_set(&cpumap, i);
> > >              n_cpus++;
> > >          }
> > > +    } else if (!xlu_cfg_get_string(config, "cpus", &buf, 0)) {
> > > +        if (cpurange_parse(buf, &cpumap))
> > 
> > Perhaps print something to say the cpu range specified is invalid?
> > 
> It happens already inside cpurange_parse() (well, to be precise, it's
> inside update_cpumap_range() ), which is in a better position for
> printing the most meaningful error messages.
> 
> If you check any other call site of cpurange_parse(), none prints
> anything on failure, exactly for that reason.

OK then.

Wei.

> 
> Regards,
> Dario

[PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

with the rest of the file, where we return 1 on 0, rather
than using libxl error codes.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 tools/libxl/xl_cmdimpl.c |   69 ++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e6d1234..14b5a3d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7034,7 +7034,7 @@ int main_cpupoolcreate(int argc, char **argv)
     libxl_bitmap cpumap;
     libxl_uuid uuid;
     libxl_cputopology *topology;
-    int rc = -ERROR_FAIL;
+    int rc = 1;
 
     SWITCH_FOREACH_OPT(opt, "hnf:", opts, "cpupool-create", 0) {
     case 'f':
@@ -7228,7 +7228,6 @@ int main_cpupoollist(int argc, char **argv)
     int n_pools, p, c, n;
     uint32_t poolid;
     char *name;
-    int ret = 0;
 
     SWITCH_FOREACH_OPT(opt, "hc", opts, "cpupool-list", 0) {
     case 'c':
@@ -7240,14 +7239,14 @@ int main_cpupoollist(int argc, char **argv)
         pool = argv[optind];
         if (libxl_name_to_cpupoolid(ctx, pool, &poolid)) {
             fprintf(stderr, "Pool \'%s\' does not exist\n", pool);
-            return -ERROR_FAIL;
+            return 1;
         }
     }
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return -ERROR_NOMEM;
+        return 1;
     }
 
     printf("%-19s", "Name");
@@ -7257,7 +7256,7 @@ int main_cpupoollist(int argc, char **argv)
         printf("CPUs   Sched     Active   Domain count\n");
 
     for (p = 0; p < n_pools; p++) {
-        if (!ret && (!pool || (poolinfo[p].poolid == poolid))) {
+        if (!pool || (poolinfo[p].poolid == poolid)) {
             name = poolinfo[p].pool_name;
             printf("%-19s", name);
             n = 0;
@@ -7278,7 +7277,7 @@ int main_cpupoollist(int argc, char **argv)
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
 
-    return ret;
+    return 0;
 }
 
 int main_cpupooldestroy(int argc, char **argv)
@@ -7295,11 +7294,14 @@ int main_cpupooldestroy(int argc, char **argv)
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
-    return -libxl_cpupool_destroy(ctx, poolid);
+    if (libxl_cpupool_destroy(ctx, poolid))
+        return 1;
+
+    return 0;
 }
 
 int main_cpupoolrename(int argc, char **argv)
@@ -7317,14 +7319,14 @@ int main_cpupoolrename(int argc, char **argv)
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
     new_name = argv[optind];
 
     if (libxl_cpupool_rename(ctx, new_name, poolid)) {
-        fprintf(stderr, "Can't rename cpupool '%s'.\n", pool);
+        fprintf(stderr, "Can't rename cpupool '%s'\n", pool);
         return 1;
     }
 
@@ -7426,22 +7428,25 @@ int main_cpupoolmigrate(int argc, char **argv)
 
     if (libxl_domain_qualifier_to_domid(ctx, dom, &domid) ||
         !libxl_domid_to_name(ctx, domid)) {
-        fprintf(stderr, "unknown domain \'%s\'\n", dom);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown domain '%s'\n", dom);
+        return 1;
     }
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
-        fprintf(stderr, "unknown cpupool \'%s\'\n", pool);
-        return -ERROR_FAIL;
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return 1;
     }
 
-    return -libxl_cpupool_movedomain(ctx, poolid, domid);
+    if (libxl_cpupool_movedomain(ctx, poolid, domid))
+        return 1;
+
+    return 0;
 }
 
 int main_cpupoolnumasplit(int argc, char **argv)
 {
-    int ret;
+    int rc;
     int opt;
     int p;
     int c;
@@ -7462,13 +7467,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
         /* No options */
     }
 
-    ret = 0;
+    rc = 1;
 
     libxl_bitmap_init(&cpumap);
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return -ERROR_NOMEM;
+        return 1;
     }
     poolid = poolinfo[0].poolid;
     sched = poolinfo[0].sched;
@@ -7477,19 +7482,19 @@ int main_cpupoolnumasplit(int argc, char **argv)
     }
     if (n_pools > 1) {
         fprintf(stderr, "splitting not possible, already cpupools in use\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     topology = libxl_get_cpu_topology(ctx, &n_cpus);
     if (topology == NULL) {
         fprintf(stderr, "libxl_get_topologyinfo failed\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Failed to allocate cpumap\n");
         libxl_cputopology_list_free(topology, n_cpus);
-        return -ERROR_FAIL;
+        return 1;
     }
 
     /* Reset Pool-0 to 1st node: first add cpus, then remove cpus to avoid
@@ -7498,12 +7503,11 @@ int main_cpupoolnumasplit(int argc, char **argv)
     node = topology[0].node;
     if (libxl_cpupool_cpuadd_node(ctx, 0, node, &n)) {
         fprintf(stderr, "error on adding cpu to Pool 0\n");
-        return -ERROR_FAIL;
+        return 1;
     }
 
     snprintf(name, 15, "Pool-node%d", node);
-    ret = -libxl_cpupool_rename(ctx, name, 0);
-    if (ret) {
+    if (libxl_cpupool_rename(ctx, name, 0)) {
         fprintf(stderr, "error on renaming Pool 0\n");
         goto out;
     }
@@ -7542,8 +7546,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
         }
 
         node = topology[c].node;
-        ret = -libxl_cpupool_cpuremove_node(ctx, 0, node, &n);
-        if (ret) {
+        if (libxl_cpupool_cpuremove_node(ctx, 0, node, &n)) {
             fprintf(stderr, "error on removing cpu from Pool 0\n");
             goto out;
         }
@@ -7551,14 +7554,12 @@ int main_cpupoolnumasplit(int argc, char **argv)
         snprintf(name, 15, "Pool-node%d", node);
         libxl_uuid_generate(&uuid);
         poolid = 0;
-        ret = -libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid);
-        if (ret) {
+        if (libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid)) {
             fprintf(stderr, "error on creating cpupool\n");
             goto out;
         }
 
-        ret = -libxl_cpupool_cpuadd_node(ctx, poolid, node, &n);
-        if (ret) {
+        if (libxl_cpupool_cpuadd_node(ctx, poolid, node, &n)) {
             fprintf(stderr, "error on adding cpus to cpupool\n");
             goto out;
         }
@@ -7570,11 +7571,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
         }
     }
 
+    rc = 0;
+
 out:
     libxl_cputopology_list_free(topology, n_cpus);
     libxl_bitmap_dispose(&cpumap);
 
-    return ret;
+    return rc;
 }
 
 int main_getenforce(int argc, char **argv)

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Wei Liu]

On Fri, Mar 06, 2015 at 06:21:59PM +0100, Dario Faggioli wrote:
> with the rest of the file, where we return 1 on 0, rather
> than using libxl error codes.
> 

While being consistent is good I'm not very sure if we should go for 0/1
rather than libxl error codes. I vaguely remember at some point we
discussed we should make xl exit code better but I don't remember the
exact details.

Ian and Ian, what do you think?

Wei.

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

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

On Mon, 2015-03-09 at 11:01 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:59PM +0100, Dario Faggioli wrote:
> > with the rest of the file, where we return 1 on 0, rather
> > than using libxl error codes.
> > 
> 
> While being consistent is good I'm not very sure if we should go for 0/1
> rather than libxl error codes. 
>
If there is a plan to switch everything in xl to use libxl error code,
then just let me know, ad I'll drop this patch.

As far as I could see, there is __A__ __LOT__ of 'return 0/1' (or
'exit(1)'), and only these few libxl error code usage, so it's really
inconsistent! :-)

> I vaguely remember at some point we
> discussed we should make xl exit code better but I don't remember the
> exact details.
> 
As I said, just let me know.

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 8/9] xl: make error reporting of cpupool subcommands consistent

[Ian Campbell]

On Mon, 2015-03-09 at 11:01 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:59PM +0100, Dario Faggioli wrote:
> > with the rest of the file, where we return 1 on 0, rather
> > than using libxl error codes.
> > 
> 
> While being consistent is good I'm not very sure if we should go for 0/1
> rather than libxl error codes. I vaguely remember at some point we
> discussed we should make xl exit code better but I don't remember the
> exact details.
> 
> Ian and Ian, what do you think?

TBH I'm not sure what exit() called with a negative number even results
in.

I think having more consistent exist codes from xl would be nice, but I
don't think the libxl error codes are the ones to use, since they don't
really map semantically onto what I would expect a CLI tool to fail with
(I'm not sure what I would expect though, something a bit higher level
on a command specific basis probably).

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Wei Liu]

On Wed, Mar 11, 2015 at 02:52:31PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-09 at 11:01 +0000, Wei Liu wrote:
> > On Fri, Mar 06, 2015 at 06:21:59PM +0100, Dario Faggioli wrote:
> > > with the rest of the file, where we return 1 on 0, rather
> > > than using libxl error codes.
> > > 
> > 
> > While being consistent is good I'm not very sure if we should go for 0/1
> > rather than libxl error codes. I vaguely remember at some point we
> > discussed we should make xl exit code better but I don't remember the
> > exact details.
> > 
> > Ian and Ian, what do you think?
> 
> TBH I'm not sure what exit() called with a negative number even results
> in.
> 
> I think having more consistent exist codes from xl would be nice, but I
> don't think the libxl error codes are the ones to use, since they don't
> really map semantically onto what I would expect a CLI tool to fail with
> (I'm not sure what I would expect though, something a bit higher level
> on a command specific basis probably).
> 

I agree that libxl error codes are not the ones to use.

Since we haven't explicitly defined any return value in xl manpage, I
think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). They are
more appropriate then 0 and 1.

Wei.

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

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

On Wed, 2015-03-11 at 15:04 +0000, Wei Liu wrote:
> On Wed, Mar 11, 2015 at 02:52:31PM +0000, Ian Campbell wrote:

> > I think having more consistent exist codes from xl would be nice, but I
> > don't think the libxl error codes are the ones to use, since they don't
> > really map semantically onto what I would expect a CLI tool to fail with
> > (I'm not sure what I would expect though, something a bit higher level
> > on a command specific basis probably).
> > 
> 
> I agree that libxl error codes are not the ones to use.
> 
> Since we haven't explicitly defined any return value in xl manpage, I
> think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). 
>
I like the idea.

> They are
> more appropriate then 0 and 1.
> 
Indeed. However, as far as this patch is concerned, what should I do?

 a) drop it, and leave libxl error code in place, until we convert 
    everything to EXIT_SUCCESS/FAILURE
 b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later
    patch(es)
 c) turn libxl error codes into EXIT_SUCCESS/FAILURE at least for this
    functions, since I'm touching them, the rest will come with later
    patch(es)
?

My opinion, I don't like c), so I'd go for either a) or b), with a
slight preference for b).

Let me know...

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 8/9] xl: make error reporting of cpupool subcommands consistent

[Wei Liu]

On Wed, Mar 11, 2015 at 03:13:33PM +0000, Dario Faggioli wrote:
> On Wed, 2015-03-11 at 15:04 +0000, Wei Liu wrote:
> > On Wed, Mar 11, 2015 at 02:52:31PM +0000, Ian Campbell wrote:
> 
> > > I think having more consistent exist codes from xl would be nice, but I
> > > don't think the libxl error codes are the ones to use, since they don't
> > > really map semantically onto what I would expect a CLI tool to fail with
> > > (I'm not sure what I would expect though, something a bit higher level
> > > on a command specific basis probably).
> > > 
> > 
> > I agree that libxl error codes are not the ones to use.
> > 
> > Since we haven't explicitly defined any return value in xl manpage, I
> > think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). 
> >
> I like the idea.
> 
> > They are
> > more appropriate then 0 and 1.
> > 
> Indeed. However, as far as this patch is concerned, what should I do?
> 
>  a) drop it, and leave libxl error code in place, until we convert 
>     everything to EXIT_SUCCESS/FAILURE
>  b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later
>     patch(es)
>  c) turn libxl error codes into EXIT_SUCCESS/FAILURE at least for this
>     functions, since I'm touching them, the rest will come with later
>     patch(es)
> ?
> 
> My opinion, I don't like c), so I'd go for either a) or b), with a
> slight preference for b).
> 

I think b) is good.

> Let me know...
> 
> Regards,
> Dario

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

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

On Wed, 2015-03-11 at 15:23 +0000, Wei Liu wrote:
> On Wed, Mar 11, 2015 at 03:13:33PM +0000, Dario Faggioli wrote:
> > On Wed, 2015-03-11 at 15:04 +0000, Wei Liu wrote:

> > > Since we haven't explicitly defined any return value in xl manpage, I
> > > think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). 
> > >
> > I like the idea.
> > 
> > > They are
> > > more appropriate then 0 and 1.
> > > 
> > Indeed. However, as far as this patch is concerned, what should I do?
> > 
> >  b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later
> >     patch(es)

> > ?
> > 
> > My opinion, I don't like c), so I'd go for either a) or b), with a
> > slight preference for b).
> > 
> 
> I think b) is good.
> 
Right. It was the only comment you made on this patch... does that means
I can stick your Acked/Reviewed -by tag to it when sending v2?

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 8/9] xl: make error reporting of cpupool subcommands consistent

[Wei Liu]

On Wed, Mar 11, 2015 at 04:22:00PM +0000, Dario Faggioli wrote:
> On Wed, 2015-03-11 at 15:23 +0000, Wei Liu wrote:
> > On Wed, Mar 11, 2015 at 03:13:33PM +0000, Dario Faggioli wrote:
> > > On Wed, 2015-03-11 at 15:04 +0000, Wei Liu wrote:
> 
> > > > Since we haven't explicitly defined any return value in xl manpage, I
> > > > think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). 
> > > >
> > > I like the idea.
> > > 
> > > > They are
> > > > more appropriate then 0 and 1.
> > > > 
> > > Indeed. However, as far as this patch is concerned, what should I do?
> > > 
> > >  b) keep it, we'll convert the 0/1 to EXIT_SUCCESS/FAILURE in later
> > >     patch(es)
> 
> > > ?
> > > 
> > > My opinion, I don't like c), so I'd go for either a) or b), with a
> > > slight preference for b).
> > > 
> > 
> > I think b) is good.
> > 
> Right. It was the only comment you made on this patch... does that means
> I can stick your Acked/Reviewed -by tag to it when sending v2?
> 

Yes. 

Acked-by: Wei Liu <wei.liu2@citrix.com>

> Dario

Re: [PATCH 8/9] xl: make error reporting of cpupool subcommands consistent

[Dario Faggioli]

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

On Wed, 2015-03-11 at 16:24 +0000, Wei Liu wrote:
> On Wed, Mar 11, 2015 at 04:22:00PM +0000, Dario Faggioli wrote:
> > On Wed, 2015-03-11 at 15:23 +0000, Wei Liu wrote:

> > > I think b) is good.
> > > 
> > Right. It was the only comment you made on this patch... does that means
> > I can stick your Acked/Reviewed -by tag to it when sending v2?
> > 
> 
> Yes. 
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
Thanks! :-)

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 8/9] xl: make error reporting of cpupool subcommands consistent

[Ian Campbell]

On Wed, 2015-03-11 at 15:04 +0000, Wei Liu wrote:
> On Wed, Mar 11, 2015 at 02:52:31PM +0000, Ian Campbell wrote:
> > On Mon, 2015-03-09 at 11:01 +0000, Wei Liu wrote:
> > > On Fri, Mar 06, 2015 at 06:21:59PM +0100, Dario Faggioli wrote:
> > > > with the rest of the file, where we return 1 on 0, rather
> > > > than using libxl error codes.
> > > > 
> > > 
> > > While being consistent is good I'm not very sure if we should go for 0/1
> > > rather than libxl error codes. I vaguely remember at some point we
> > > discussed we should make xl exit code better but I don't remember the
> > > exact details.
> > > 
> > > Ian and Ian, what do you think?
> > 
> > TBH I'm not sure what exit() called with a negative number even results
> > in.
> > 
> > I think having more consistent exist codes from xl would be nice, but I
> > don't think the libxl error codes are the ones to use, since they don't
> > really map semantically onto what I would expect a CLI tool to fail with
> > (I'm not sure what I would expect though, something a bit higher level
> > on a command specific basis probably).
> > 
> 
> I agree that libxl error codes are not the ones to use.
> 
> Since we haven't explicitly defined any return value in xl manpage, I
> think we should use EXIT_SUCCESS and EXIT_FAILURE per exit(3). They are
> more appropriate then 0 and 1.

I guess so, yes.

Ian.

[PATCH 9/9] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

[Dario Faggioli]

instead manually free the elements of the list, which is
exactly the purpose of the said function.

Trade also a couple of 'return'-s with 'goto out'-s, which
is more in line with libxl usage paradigm.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Juergen Gross <JGross@suse.com>
---
 tools/libxl/xl_cmdimpl.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 14b5a3d..ec2b6a9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7477,9 +7477,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
     }
     poolid = poolinfo[0].poolid;
     sched = poolinfo[0].sched;
-    for (p = 0; p < n_pools; p++) {
-        libxl_cpupoolinfo_dispose(poolinfo + p);
-    }
+    libxl_cpupoolinfo_list_free(poolinfo, n_pools);
+
     if (n_pools > 1) {
         fprintf(stderr, "splitting not possible, already cpupools in use\n");
         return 1;
@@ -7493,8 +7492,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Failed to allocate cpumap\n");
-        libxl_cputopology_list_free(topology, n_cpus);
-        return 1;
+        goto out;
     }
 
     /* Reset Pool-0 to 1st node: first add cpus, then remove cpus to avoid
@@ -7503,7 +7501,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
     node = topology[0].node;
     if (libxl_cpupool_cpuadd_node(ctx, 0, node, &n)) {
         fprintf(stderr, "error on adding cpu to Pool 0\n");
-        return 1;
+        goto out;
     }
 
     snprintf(name, 15, "Pool-node%d", node);

Re: [PATCH 9/9] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

[Wei Liu]

On Fri, Mar 06, 2015 at 06:22:07PM +0100, Dario Faggioli wrote:
> instead manually free the elements of the list, which is
> exactly the purpose of the said function.
> 
> Trade also a couple of 'return'-s with 'goto out'-s, which
> is more in line with libxl usage paradigm.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH 9/9] xl: use libxl_cpupoolinfo_list_free() in main_cpupoolnumasplit

[Ian Campbell]

On Fri, 2015-03-06 at 18:22 +0100, Dario Faggioli wrote:
> instead manually free the elements of the list, which is
         ^of       freeing

> exactly the purpose of the said function.
> 
> Trade also a couple of 'return'-s with 'goto out'-s, which
> is more in line with libxl usage paradigm.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Juergen Gross <JGross@suse.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 14b5a3d..ec2b6a9 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7477,9 +7477,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
>      }
>      poolid = poolinfo[0].poolid;
>      sched = poolinfo[0].sched;
> -    for (p = 0; p < n_pools; p++) {
> -        libxl_cpupoolinfo_dispose(poolinfo + p);
> -    }
> +    libxl_cpupoolinfo_list_free(poolinfo, n_pools);
> +
>      if (n_pools > 1) {
>          fprintf(stderr, "splitting not possible, already cpupools in use\n");
>          return 1;
> @@ -7493,8 +7492,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
>  
>      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
>          fprintf(stderr, "Failed to allocate cpumap\n");
> -        libxl_cputopology_list_free(topology, n_cpus);
> -        return 1;
> +        goto out;
>      }
>  
>      /* Reset Pool-0 to 1st node: first add cpus, then remove cpus to avoid
> @@ -7503,7 +7501,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
>      node = topology[0].node;
>      if (libxl_cpupool_cpuadd_node(ctx, 0, node, &n)) {
>          fprintf(stderr, "error on adding cpu to Pool 0\n");
> -        return 1;
> +        goto out;
>      }
>  
>      snprintf(name, 15, "Pool-node%d", node);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/9] Some (not only) cpupool related fixes and improvements

[Ian Campbell]

On Fri, 2015-03-06 at 18:20 +0100, Dario Faggioli wrote:
> Hi everyone,
> 
> The main goal of this series is making it possible to specify ranges of pCPUs
> when manipulating (creating, adding/removing pCPUs) cpupools. Something like
> this:
> 
>  xl cpupool-cpu-remove Pool-node0 6-10
> 
> while, right now, only single pCPU IDs can be specified, which means that, to
> achieve the above, we need to do this:
> 
>  xl cpupool-cpu-remove Pool-node0 6
>  xl cpupool-cpu-remove Pool-node0 7
>  xl cpupool-cpu-remove Pool-node0 8
>  xl cpupool-cpu-remove Pool-node0 9
>  xl cpupool-cpu-remove Pool-node0 10
> 
> This is done in patches 5/9, 6/9 and 7/9.
> 
> The series also add a new parameter to `xl list', '-c' or '--cpupool', which
> shows in which cpupool the domain(s) lives. Such information was not easy to
> find in any other way. One can use `xl sched-<schedule_of_cpupool_x>' and have,
> as a side effect, something similar, but that's rather impractical.
> 
> Sample output for the new `xl list' can be found here:
> 
>  http://pastebin.com/qSagPimL
> 
> This happens in patch 3/9 and 4/9.
> 
> While there, the series includes some other, somewhat related, fixes of various
> kind.
> 
> This is available as a git branch here:
> 
>  git://xenbits.xen.org/people/dariof/xen.git rel/cpupools/allow-ranges-v1
>  http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/cpupools/allow-ranges-v1
> 
> 
> Thanks and Regards,
> Dario
> 
> ---
> Dario Faggioli (9):
>   1/9  docs: RTDS is a valid alternative as a scheduler for a cpupool
>   2/9  docs: fix `xl list' manpage entry

Acked + applied these two.

>   3/9  xl: turn some int local variable into bool

I'd have applied this with Wei's ack but he latter noticed that some 0/1
hadn't been converted so I thought I'd give you the chance to update if
you wanted.

For the rest I think there are outstanding comments to be addressed.

Re: [PATCH 0/9] Some (not only) cpupool related fixes and improvements

[Dario Faggioli]

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

On Wed, 2015-03-11 at 14:56 +0000, Ian Campbell wrote:
> On Fri, 2015-03-06 at 18:20 +0100, Dario Faggioli wrote:

> > ---
> > Dario Faggioli (9):
> >   1/9  docs: RTDS is a valid alternative as a scheduler for a cpupool
> >   2/9  docs: fix `xl list' manpage entry
> 
> Acked + applied these two.
> 
Thanks.

> >   3/9  xl: turn some int local variable into bool
> 
> I'd have applied this with Wei's ack but he latter noticed that some 0/1
> hadn't been converted so I thought I'd give you the chance to update if
> you wanted.
> 
I am preparing v2, I was just waiting to hear from you and/or Ian about
the xl exist code bit.

> For the rest I think there are outstanding comments to be addressed.
> 
They are. Will send v2.

Thanks again and 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