[PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Ian Jackson]

This adjust commit a49077e5 "Fix segfaults from `xl psr-cat-cbm-set`
command line handling":

 * Do not use the constant `required_argument' here (we simply use 1
   everywhere else).

 * Fix the minimum required arguments argument to SWITCH_FOREACH_OPT.

Leave the separate check on optind, because it checks for too many as
well as too few arguments.

(There are many things in xl which fail to check for too many
arguments.  I do not intend to drain that swamp now: I started but
decided a complete overhaul of most of xl's command line argument
processing would be best.)

This is just a code cleanup with no ultimate functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Chao Peng <chao.p.peng@linux.intel.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7949202..55c041c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8397,7 +8397,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     int i, j, len;
 
     static struct option opts[] = {
-        {"socket", required_argument, 0, 's'},
+        {"socket", 1, 0, 's'},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
@@ -8405,7 +8405,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     libxl_socket_bitmap_alloc(ctx, &target_map, 0);
     libxl_bitmap_set_none(&target_map);
 
-    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 1) {
+    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 2) {
     case 's':
         trim(isspace, optarg, &value);
         split_string_into_string_list(value, ",", &socket_list);
-- 
1.7.10.4

[PATCH 2/5] xl: Command line: Remove maximum argument limit for network-attach

[Ian Jackson]

This limit of 11 has been in this function since it was written, but
serves no purpose.  The extra arguments are fed one by one to
parse_nic_config, and it is possible to have as many as you like.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 55c041c..770b71c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6373,11 +6373,6 @@ int main_networkattach(int argc, char **argv)
         /* No options */
     }
 
-    if (argc-optind > 11) {
-        help("network-attach");
-        return 0;
-    }
-
     domid = find_domain(argv[optind]);
 
     config= xlu_cfg_init(stderr, "command line");
-- 
1.7.10.4

Re: [PATCH 2/5] xl: Command line: Remove maximum argument limit for network-attach

[Wei Liu]

On Fri, Jul 17, 2015 at 06:00:48PM +0100, Ian Jackson wrote:
> This limit of 11 has been in this function since it was written, but
> serves no purpose.  The extra arguments are fed one by one to
> parse_nic_config, and it is possible to have as many as you like.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

> ---
>  tools/libxl/xl_cmdimpl.c |    5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 55c041c..770b71c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -6373,11 +6373,6 @@ int main_networkattach(int argc, char **argv)
>          /* No options */
>      }
>  
> -    if (argc-optind > 11) {
> -        help("network-attach");
> -        return 0;
> -    }
> -
>      domid = find_domain(argv[optind]);
>  
>      config= xlu_cfg_init(stderr, "command line");
> -- 
> 1.7.10.4

[PATCH 3/5] xl: Command line: Support -h everywhere

[Ian Jackson]

xl subcommands ought all to take -h.  def_getopt and hence
SWITCH_FOREACH_OPT already handles 'h' by calling helpstr.  None of
the call sites see the 'h'.

In this patch:

 * Change SWITCH_FOREACH_OPT to always add a "h" to the short opts
   string, using string concatenation.

 * Remove the now-redundant h's from some existing option strings.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 770b71c..803c627 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3073,14 +3073,15 @@ static int def_getopt(int argc, char * const argv[],
  * Wraps def_getopt into a convenient loop+switch to process all
  * arguments. This macro is intended to be called from main_XXX().
  *
- *   SWITCH_FOREACH_OPT(int *opt, const char *opts,
+ *   SWITCH_FOREACH_OPT(int *opt, "OPTS",
  *                      const struct option *longopts,
  *                      const char *commandname,
  *                      int num_opts_req) { ...
  *
  * opt:               pointer to an int variable, holds the current option
  *                    during processing.
- * opts:              short options, as per getopt_long(3)'s optstring argument.
+ * OPTS:              short options, as per getopt_long(3)'s optstring argument.
+ *                    do not include "h"; will be provided automatically
  * longopts:          long options, as per getopt_long(3)'s longopts argument.
  *                    May be null.
  * commandname:       name of this command, for usage string.
@@ -3124,7 +3125,7 @@ static int def_getopt(int argc, char * const argv[],
  */
 #define SWITCH_FOREACH_OPT(opt, opts, longopts,                         \
                            commandname, num_required_opts)              \
-    while (((opt) = def_getopt(argc, argv, (opts), (longopts),          \
+    while (((opt) = def_getopt(argc, argv, "h" opts, (longopts),          \
                                 (commandname), (num_required_opts))) != -1) \
         switch (opt)
 
@@ -3310,7 +3311,7 @@ int main_vncviewer(int argc, char **argv)
     uint32_t domid;
     int opt, autopass = 0;
 
-    SWITCH_FOREACH_OPT(opt, "ah", opts, "vncviewer", 1) {
+    SWITCH_FOREACH_OPT(opt, "a", opts, "vncviewer", 1) {
     case 'a':
         autopass = 1;
         break;
@@ -4442,7 +4443,7 @@ int main_restore(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
-    SWITCH_FOREACH_OPT(opt, "FhcpdeVA", opts, "restore", 1) {
+    SWITCH_FOREACH_OPT(opt, "FcpdeVA", opts, "restore", 1) {
     case 'c':
         console_autoconnect = 1;
         break;
@@ -4893,7 +4894,7 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "Fhnqf:pcdeVA", opts, "create", 0) {
+    SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVA", opts, "create", 0) {
     case 'f':
         filename = optarg;
         break;
@@ -4997,7 +4998,7 @@ int main_config_update(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "dhqf:", opts, "config_update", 0) {
+    SWITCH_FOREACH_OPT(opt, "dqf:", opts, "config_update", 0) {
     case 'd':
         debug = 1;
         break;
@@ -5588,7 +5589,7 @@ int main_info(int argc, char **argv)
     };
     int numa = 0;
 
-    SWITCH_FOREACH_OPT(opt, "hn", opts, "info", 0) {
+    SWITCH_FOREACH_OPT(opt, "n", opts, "info", 0) {
     case 'n':
         numa = 1;
         break;
@@ -5926,7 +5927,7 @@ int main_sched_credit(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:w:c:p:t:r:hs", opts, "sched-credit", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:w:c:p:t:r:s", opts, "sched-credit", 0) {
     case 'd':
         dom = optarg;
         break;
@@ -6042,7 +6043,7 @@ int main_sched_credit2(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:w:p:h", opts, "sched-credit2", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:w:p:", opts, "sched-credit2", 0) {
     case 'd':
         dom = optarg;
         break;
@@ -6115,7 +6116,7 @@ int main_sched_rtds(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:p:b:c:h", opts, "sched-rtds", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
     case 'd':
         dom = optarg;
         break;
@@ -7200,7 +7201,7 @@ int main_cpupoolcreate(int argc, char **argv)
     libxl_cputopology *topology;
     int rc = 1;
 
-    SWITCH_FOREACH_OPT(opt, "hnf:", opts, "cpupool-create", 0) {
+    SWITCH_FOREACH_OPT(opt, "nf:", opts, "cpupool-create", 0) {
     case 'f':
         filename = optarg;
         break;
@@ -7391,7 +7392,7 @@ int main_cpupoollist(int argc, char **argv)
     uint32_t poolid;
     char *name;
 
-    SWITCH_FOREACH_OPT(opt, "hc", opts, "cpupool-list", 0) {
+    SWITCH_FOREACH_OPT(opt, "c", opts, "cpupool-list", 0) {
     case 'c':
         opt_cpus = 1;
         break;
-- 
1.7.10.4

Re: [PATCH 3/5] xl: Command line: Support -h everywhere

[Wei Liu]

On Fri, Jul 17, 2015 at 06:00:49PM +0100, Ian Jackson wrote:
> xl subcommands ought all to take -h.  def_getopt and hence
> SWITCH_FOREACH_OPT already handles 'h' by calling helpstr.  None of
> the call sites see the 'h'.
> 
> In this patch:
> 
>  * Change SWITCH_FOREACH_OPT to always add a "h" to the short opts
>    string, using string concatenation.
> 
>  * Remove the now-redundant h's from some existing option strings.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

[PATCH 4/5] xl: Command line: Make COMMON_LONG_OPTS include sentinel

[Ian Jackson]

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   51 ++++++++++++++++------------------------------
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 803c627..1fa98e6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3025,7 +3025,8 @@ static int64_t parse_mem_size_kb(const char *mem)
     return kbytes;
 }
 
-#define COMMON_LONG_OPTS {"help", 0, 0, 'h'}
+#define COMMON_LONG_OPTS {"help", 0, 0, 'h'}, \
+                         {0, 0, 0, 0}
 
 /*
  * Callers should use SWITCH_FOREACH_OPT in preference to calling this
@@ -3038,8 +3039,7 @@ static int def_getopt(int argc, char * const argv[],
 {
     int opt;
     const struct option def_options[] = {
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     if (!longopts)
@@ -3305,8 +3305,7 @@ int main_vncviewer(int argc, char **argv)
     static const struct option opts[] = {
         {"autopass", 0, 0, 'a'},
         {"vncviewer-autopass", 0, 0, 'a'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
     uint32_t domid;
     int opt, autopass = 0;
@@ -4439,8 +4438,7 @@ int main_restore(int argc, char **argv)
     static struct option opts[] = {
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "FcpdeVA", opts, "restore", 1) {
@@ -4572,8 +4570,7 @@ int main_migrate(int argc, char **argv)
     static struct option opts[] = {
         {"debug", 0, 0, 0x100},
         {"live", 0, 0, 0x200},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "FC:s:e", opts, "migrate", 2) {
@@ -4695,8 +4692,7 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
     static struct option opts[] = {
         {"all", 0, 0, 'a'},
         {"wait", 0, 0, 'w'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "awF", opts, what, 0) {
@@ -4776,8 +4772,7 @@ int main_list(int argc, char **argv)
         {"context", 0, 0, 'Z'},
         {"cpupool", 0, 0, 'c'},
         {"numa", 0, 0, 'n'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     libxl_dominfo info_buf;
@@ -4883,8 +4878,7 @@ int main_create(int argc, char **argv)
         {"defconfig", 1, 0, 'f'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     dom_info.extra_config = NULL;
@@ -4977,8 +4971,7 @@ int main_config_update(int argc, char **argv)
     int debug = 0;
     static struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     if (argc < 2) {
@@ -5584,8 +5577,7 @@ int main_info(int argc, char **argv)
     int opt;
     static struct option opts[] = {
         {"numa", 0, 0, 'n'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
     int numa = 0;
 
@@ -5923,8 +5915,7 @@ int main_sched_credit(int argc, char **argv)
         {"tslice_ms", 1, 0, 't'},
         {"ratelimit_us", 1, 0, 'r'},
         {"cpupool", 1, 0, 'p'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "d:w:c:p:t:r:s", opts, "sched-credit", 0) {
@@ -6039,8 +6030,7 @@ int main_sched_credit2(int argc, char **argv)
         {"domain", 1, 0, 'd'},
         {"weight", 1, 0, 'w'},
         {"cpupool", 1, 0, 'p'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "d:w:p:", opts, "sched-credit2", 0) {
@@ -6112,8 +6102,7 @@ int main_sched_rtds(int argc, char **argv)
         {"period", 1, 0, 'p'},
         {"budget", 1, 0, 'b'},
         {"cpupool", 1, 0, 'c'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
@@ -7181,8 +7170,7 @@ int main_cpupoolcreate(int argc, char **argv)
     static struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
         {"dryrun", 0, 0, 'n'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
     int ret;
     char *config_data = 0;
@@ -7382,8 +7370,7 @@ int main_cpupoollist(int argc, char **argv)
     int opt;
     static struct option opts[] = {
         {"cpus", 0, 0, 'c'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
     int opt_cpus = 0;
     const char *pool = NULL;
@@ -8394,8 +8381,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
 
     static struct option opts[] = {
         {"socket", 1, 0, 's'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     libxl_socket_bitmap_alloc(ctx, &target_map, 0);
@@ -8462,8 +8448,7 @@ int main_psr_hwinfo(int argc, char **argv)
     static struct option opts[] = {
         {"cmt", 0, 0, 'm'},
         {"cat", 0, 0, 'a'},
-        COMMON_LONG_OPTS,
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
 
     SWITCH_FOREACH_OPT(opt, "ma", opts, "psr-hwinfo", 0) {
-- 
1.7.10.4

Re: [PATCH 4/5] xl: Command line: Make COMMON_LONG_OPTS include sentinel

[Andrew Cooper]

On 17/07/15 18:00, Ian Jackson wrote:
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   51 ++++++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 803c627..1fa98e6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3025,7 +3025,8 @@ static int64_t parse_mem_size_kb(const char *mem)
>      return kbytes;
>  }
>  

Perhaps a comment stating that COMMON_LONG_OPTS must now be the final
entry in an option list?  Previously it was technically able to live
anywhere in the list.

~Andrew

> -#define COMMON_LONG_OPTS {"help", 0, 0, 'h'}
> +#define COMMON_LONG_OPTS {"help", 0, 0, 'h'}, \
> +                         {0, 0, 0, 0}
>  
>  /*
>   * Callers should use SWITCH_FOREACH_OPT in preference to calling this
>

Re: [PATCH 4/5] xl: Command line: Make COMMON_LONG_OPTS include sentinel

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 4/5] xl: Command line: Make COMMON_LONG_OPTS include sentinel"):
> On 17/07/15 18:00, Ian Jackson wrote:
> > No functional change.

> Perhaps a comment stating that COMMON_LONG_OPTS must now be the final
> entry in an option list?  Previously it was technically able to live
> anywhere in the list.

Sure.

Ian.

Re: [PATCH 4/5] xl: Command line: Make COMMON_LONG_OPTS include sentinel

[Ian Campbell]

On Fri, 2015-07-17 at 18:11 +0100, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 4/5] xl: Command line: 
> Make COMMON_LONG_OPTS include sentinel"):
> > On 17/07/15 18:00, Ian Jackson wrote:
> > > No functional change.
> 
> > Perhaps a comment stating that COMMON_LONG_OPTS must now be the 
> > final
> > entry in an option list?  Previously it was technically able to 
> > live
> > anywhere in the list.
> 
> Sure.

I applied and inserted:
/* Must be last in list */
 before the #define

I also picked up the last patch.

[PATCH 5/5] xl: Command line: Support xl vcpu-set --help

[Ian Jackson]

This ended with a literal sentinel.  Use COMMON_LONG_OPTIONS (which
mentions --help) instead.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1fa98e6..337f805 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5365,7 +5365,7 @@ int main_vcpuset(int argc, char **argv)
 {
     static struct option opts[] = {
         {"ignore-host", 0, 0, 'i'},
-        {0, 0, 0, 0}
+        COMMON_LONG_OPTS
     };
     int opt, check_host = 1;
 
-- 
1.7.10.4

Re: [PATCH 5/5] xl: Command line: Support xl vcpu-set --help

[Wei Liu]

On Fri, Jul 17, 2015 at 06:00:51PM +0100, Ian Jackson wrote:
> This ended with a literal sentinel.  Use COMMON_LONG_OPTIONS (which
> mentions --help) instead.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Andrew Cooper]

On 17/07/15 18:00, Ian Jackson wrote:
> This adjust commit a49077e5 "Fix segfaults from `xl psr-cat-cbm-set`
> command line handling":
>
>  * Do not use the constant `required_argument' here (we simply use 1
>    everywhere else).
>
>  * Fix the minimum required arguments argument to SWITCH_FOREACH_OPT.
>
> Leave the separate check on optind, because it checks for too many as
> well as too few arguments.
>
> (There are many things in xl which fail to check for too many
> arguments.  I do not intend to drain that swamp now: I started but
> decided a complete overhaul of most of xl's command line argument
> processing would be best.)
>
> This is just a code cleanup with no ultimate functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Chao Peng <chao.p.peng@linux.intel.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I had not realised about the final parameter to SWITCH_FOREACH_OPT(). 
(The patch was a fix while I was attempting to get something else working)

Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Ian Campbell]

On Fri, 2015-07-17 at 18:00 +0100, Ian Jackson wrote:

Replying here in lieu of a 0/N:

Is any subset of this aimed at 4.6?


> This adjust commit a49077e5 "Fix segfaults from `xl psr-cat-cbm-set`
> command line handling":
> 
>  * Do not use the constant `required_argument' here (we simply use 1
>    everywhere else).
> 
>  * Fix the minimum required arguments argument to SWITCH_FOREACH_OPT.
> 
> Leave the separate check on optind, because it checks for too many as
> well as too few arguments.
> 
> (There are many things in xl which fail to check for too many
> arguments.  I do not intend to drain that swamp now: I started but
> decided a complete overhaul of most of xl's command line argument
> processing would be best.)
> 
> This is just a code cleanup with no ultimate functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Chao Peng <chao.p.peng@linux.intel.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7949202..55c041c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8397,7 +8397,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      int i, j, len;
>  
>      static struct option opts[] = {
> -        {"socket", required_argument, 0, 's'},
> +        {"socket", 1, 0, 's'},
>          COMMON_LONG_OPTS,
>          {0, 0, 0, 0}
>      };
> @@ -8405,7 +8405,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      libxl_socket_bitmap_alloc(ctx, &target_map, 0);
>      libxl_bitmap_set_none(&target_map);
>  
> -    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 1) {
> +    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 2) {
>      case 's':
>          trim(isspace, optarg, &value);
>          split_string_into_string_list(value, ",", &socket_list);

Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`...""):
> On Fri, 2015-07-17 at 18:00 +0100, Ian Jackson wrote:
> 
> Replying here in lieu of a 0/N:
> 
> Is any subset of this aimed at 4.6?

Yes, ideally, all of them.  I think they are bugfixes or minor
cleanups.

If Wei wants only a subset, I can probably tease them out.

Ian.

Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Wei Liu]

On Tue, Jul 21, 2015 at 03:27:26PM +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`...""):
> > On Fri, 2015-07-17 at 18:00 +0100, Ian Jackson wrote:
> > 
> > Replying here in lieu of a 0/N:
> > 
> > Is any subset of this aimed at 4.6?
> 
> Yes, ideally, all of them.  I think they are bugfixes or minor
> cleanups.
> 
> If Wei wants only a subset, I can probably tease them out.
> 

I think this is part of the audit of toolstack code.  They look
obviously correct.  I'm fine with them going in.

Currently we have three series with freeze exception. Only RMRR has
significant number of changes for xl, however that series doens't
introduce a new command, so we're probably safe to apply this series
now.

Wei.

> Ian.

Re: [PATCH 1/5] xl: Command line: Adjust "Fix segfaults from `xl psr-cat-cbm-set`..."

[Ian Campbell]

On Tue, 2015-07-21 at 15:36 +0100, Wei Liu wrote:
> On Tue, Jul 21, 2015 at 03:27:26PM +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH 1/5] xl: Command line: Adjust "Fix 
> > segfaults from `xl psr-cat-cbm-set`...""):
> > > On Fri, 2015-07-17 at 18:00 +0100, Ian Jackson wrote:
> > > 
> > > Replying here in lieu of a 0/N:
> > > 
> > > Is any subset of this aimed at 4.6?
> > 
> > Yes, ideally, all of them.  I think they are bugfixes or minor
> > cleanups.
> > 
> > If Wei wants only a subset, I can probably tease them out.
> > 
> 
> I think this is part of the audit of toolstack code.  They look
> obviously correct.  I'm fine with them going in.
> 
> Currently we have three series with freeze exception. Only RMRR has
> significant number of changes for xl, however that series doens't
> introduce a new command, so we're probably safe to apply this series
> now.

I applied #1..3, #4 had a good suggestion for an improvement and #5
relies on that change so I left them in this pass.

Ian.