[Qemu-devel] [PATCH 0/4] linux-user: CLI cleanup

[Qemu-devel] [PATCH 0/4] linux-user: CLI cleanup

[meadori]

From: Meador Inge <meadori@codesourcery.com>

This patch series fixes a few nits in the Linux
usermode driver to make the general behavior less
surprising (proper error codes, --foo options, and
-help) and to make it easier to discover bad command
line input.

Meador Inge (4):
  linux-user: Exit 0 when -h is used
  linux-user: Add -help
  linux-user: Add proper error messages for bad options
  linux-user: Treat --foo options the same as -foo

 linux-user/main.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

-- 
1.8.1.1

[Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[meadori]

From: Meador Inge <meadori@codesourcery.com>

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/main.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index c855bcc..c6ab557 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -65,7 +65,7 @@ unsigned long reserved_va;
 #endif
 #endif
 
-static void usage(void);
+static void usage(int exitcode);
 
 static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
 const char *qemu_uname_release;
@@ -3473,7 +3473,7 @@ CPUArchState *cpu_copy(CPUArchState *env)
 
 static void handle_arg_help(const char *arg)
 {
-    usage();
+    usage(0);
 }
 
 static void handle_arg_log(const char *arg)
@@ -3499,7 +3499,7 @@ static void handle_arg_set_env(const char *arg)
     r = p = strdup(arg);
     while ((token = strsep(&p, ",")) != NULL) {
         if (envlist_setenv(envlist, token) != 0) {
-            usage();
+            usage(1);
         }
     }
     free(r);
@@ -3511,7 +3511,7 @@ static void handle_arg_unset_env(const char *arg)
     r = p = strdup(arg);
     while ((token = strsep(&p, ",")) != NULL) {
         if (envlist_unsetenv(envlist, token) != 0) {
-            usage();
+            usage(1);
         }
     }
     free(r);
@@ -3527,7 +3527,7 @@ static void handle_arg_stack_size(const char *arg)
     char *p;
     guest_stack_size = strtoul(arg, &p, 0);
     if (guest_stack_size == 0) {
-        usage();
+        usage(1);
     }
 
     if (*p == 'M') {
@@ -3698,7 +3698,7 @@ static const struct qemu_argument arg_table[] = {
     {NULL, NULL, false, NULL, NULL, NULL}
 };
 
-static void usage(void)
+static void usage(int exitcode)
 {
     const struct qemu_argument *arginfo;
     int maxarglen;
@@ -3765,7 +3765,7 @@ static void usage(void)
            "Note that if you provide several changes to a single variable\n"
            "the last change will stay in effect.\n");
 
-    exit(1);
+    exit(exitcode);
 }
 
 static int parse_args(int argc, char **argv)
@@ -3804,7 +3804,7 @@ static int parse_args(int argc, char **argv)
             if (!strcmp(r, arginfo->argv)) {
                 if (arginfo->has_arg) {
                     if (optind >= argc) {
-                        usage();
+                        usage(1);
                     }
                     arginfo->handle_opt(argv[optind]);
                     optind++;
@@ -3817,12 +3817,12 @@ static int parse_args(int argc, char **argv)
 
         /* no option matched the current argv */
         if (arginfo->handle_opt == NULL) {
-            usage();
+            usage(1);
         }
     }
 
     if (optind >= argc) {
-        usage();
+        usage(1);
     }
 
     filename = argv[optind];
-- 
1.8.1.1

[Qemu-devel] [PATCH 2/4] linux-user: Add -help

[meadori]

From: Meador Inge <meadori@codesourcery.com>

This option is already available on the system mode
binaries.  It would be better if long options were
supported (i.e. --help), but this is okay for now.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index c6ab557..2922d23 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3658,6 +3658,8 @@ struct qemu_argument {
 static const struct qemu_argument arg_table[] = {
     {"h",          "",                 false, handle_arg_help,
      "",           "print this help"},
+    {"help",       "",                 false, handle_arg_help,
+     "",           ""},
     {"g",          "QEMU_GDB",         true,  handle_arg_gdb,
      "port",       "wait gdb connection to 'port'"},
     {"L",          "QEMU_LD_PREFIX",   true,  handle_arg_ld_prefix,
-- 
1.8.1.1

[Qemu-devel] [PATCH 3/4] linux-user: Add proper error messages for bad options

[meadori]

From: Meador Inge <meadori@codesourcery.com>

This patch adds better support for diagnosing option
parser errors.  The previous implementation just printed
the usage text and exited when a bad option or argument
was found.  This made it very difficult to determine why
the usage was being displayed and it was doubly confusing
for cases like '--help' (it wasn't clear that --help was
actually an error).

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 2922d23..94badfc 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3806,7 +3806,9 @@ static int parse_args(int argc, char **argv)
             if (!strcmp(r, arginfo->argv)) {
                 if (arginfo->has_arg) {
                     if (optind >= argc) {
-                        usage(1);
+                        (void) fprintf(stderr,
+                            "qemu: missing argument for option '%s'\n", r);
+                        exit(1);
                     }
                     arginfo->handle_opt(argv[optind]);
                     optind++;
@@ -3819,12 +3821,14 @@ static int parse_args(int argc, char **argv)
 
         /* no option matched the current argv */
         if (arginfo->handle_opt == NULL) {
-            usage(1);
+            (void) fprintf(stderr, "qemu: unknown option '%s'\n", r);
+            exit(1);
         }
     }
 
     if (optind >= argc) {
-        usage(1);
+        (void) fprintf(stderr, "qemu: no user program specified\n");
+        exit(1);
     }
 
     filename = argv[optind];
-- 
1.8.1.1

[Qemu-devel] [PATCH 4/4] linux-user: Treat --foo options the same as -foo

[meadori]

From: Meador Inge <meadori@codesourcery.com>

The system mode binaries provide a similar alias
and it makes common options like --version and --help
work as expected.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 94badfc..adf6fb1 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3801,6 +3801,10 @@ static int parse_args(int argc, char **argv)
         if (!strcmp(r, "-")) {
             break;
         }
+        /* Treat --foo the same as -foo.  */
+        if (r[0] == '-') {
+            r++;
+        }
 
         for (arginfo = arg_table; arginfo->handle_opt != NULL; arginfo++) {
             if (!strcmp(r, arginfo->argv)) {
-- 
1.8.1.1

Re: [Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Laurent Vivier]

Global comment: you should use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h


Le 06/07/2015 20:03, meadori@codesourcery.com a écrit :
> From: Meador Inge <meadori@codesourcery.com>
> 
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  linux-user/main.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index c855bcc..c6ab557 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -65,7 +65,7 @@ unsigned long reserved_va;
>  #endif
>  #endif
>  
> -static void usage(void);
> +static void usage(int exitcode);
>  
>  static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
>  const char *qemu_uname_release;
> @@ -3473,7 +3473,7 @@ CPUArchState *cpu_copy(CPUArchState *env)
>  
>  static void handle_arg_help(const char *arg)
>  {
> -    usage();
> +    usage(0);
>  }
>  
>  static void handle_arg_log(const char *arg)
> @@ -3499,7 +3499,7 @@ static void handle_arg_set_env(const char *arg)
>      r = p = strdup(arg);
>      while ((token = strsep(&p, ",")) != NULL) {
>          if (envlist_setenv(envlist, token) != 0) {
> -            usage();
> +            usage(1);
>          }
>      }
>      free(r);
> @@ -3511,7 +3511,7 @@ static void handle_arg_unset_env(const char *arg)
>      r = p = strdup(arg);
>      while ((token = strsep(&p, ",")) != NULL) {
>          if (envlist_unsetenv(envlist, token) != 0) {
> -            usage();
> +            usage(1);
>          }
>      }
>      free(r);
> @@ -3527,7 +3527,7 @@ static void handle_arg_stack_size(const char *arg)
>      char *p;
>      guest_stack_size = strtoul(arg, &p, 0);
>      if (guest_stack_size == 0) {
> -        usage();
> +        usage(1);
>      }
>  
>      if (*p == 'M') {
> @@ -3698,7 +3698,7 @@ static const struct qemu_argument arg_table[] = {
>      {NULL, NULL, false, NULL, NULL, NULL}
>  };
>  
> -static void usage(void)
> +static void usage(int exitcode)
>  {
>      const struct qemu_argument *arginfo;
>      int maxarglen;
> @@ -3765,7 +3765,7 @@ static void usage(void)
>             "Note that if you provide several changes to a single variable\n"
>             "the last change will stay in effect.\n");
>  
> -    exit(1);
> +    exit(exitcode);
>  }
>  
>  static int parse_args(int argc, char **argv)
> @@ -3804,7 +3804,7 @@ static int parse_args(int argc, char **argv)
>              if (!strcmp(r, arginfo->argv)) {
>                  if (arginfo->has_arg) {
>                      if (optind >= argc) {
> -                        usage();
> +                        usage(1);
>                      }
>                      arginfo->handle_opt(argv[optind]);
>                      optind++;
> @@ -3817,12 +3817,12 @@ static int parse_args(int argc, char **argv)
>  
>          /* no option matched the current argv */
>          if (arginfo->handle_opt == NULL) {
> -            usage();
> +            usage(1);
>          }
>      }
>  
>      if (optind >= argc) {
> -        usage();
> +        usage(1);
>      }
>  
>      filename = argv[optind];
> 

Re: [Qemu-devel] [PATCH 0/4] linux-user: CLI cleanup

[Laurent Vivier]

Le 06/07/2015 20:03, meadori@codesourcery.com a écrit :
> From: Meador Inge <meadori@codesourcery.com>
> 
> This patch series fixes a few nits in the Linux
> usermode driver to make the general behavior less
> surprising (proper error codes, --foo options, and
> -help) and to make it easier to discover bad command
> line input.
> 
> Meador Inge (4):
>   linux-user: Exit 0 when -h is used
>   linux-user: Add -help
>   linux-user: Add proper error messages for bad options
>   linux-user: Treat --foo options the same as -foo
> 
>  linux-user/main.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 

Perhaps it is time to switch to getopt_long() ?
(We could manage environment variable in second array).

Riku, your opinion ?

Laurent

Re: [Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Meador Inge]

On Mon, Jul 06, 2015 at 09:43:20PM +0200, Laurent Vivier wrote:

> Global comment: you should use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h

Will fix.  Thanks.

-- Meador

Re: [Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Meador Inge]

On Mon, Jul 06, 2015 at 09:43:20PM +0200, Laurent Vivier wrote:

> Global comment: you should use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h

On second thought, I was following an existing pattern in 'main.c'. Really
fixing this would require changing around 30 other locations too.  I think
if we are going to switch to the EXIT_* macros, then the whole file should
be changed in one patch and making that fix shouldn't hold this patch up.

-- Meador

Re: [Qemu-devel] [PATCH 0/4] linux-user: CLI cleanup

[Riku Voipio]

On Mon, Jul 06, 2015 at 11:03:37AM -0700, meadori@codesourcery.com wrote:
> From: Meador Inge <meadori@codesourcery.com>
> 
> This patch series fixes a few nits in the Linux
> usermode driver to make the general behavior less
> surprising (proper error codes, --foo options, and
> -help) and to make it easier to discover bad command
> line input.

Applied to linux-user, thanks.

> Meador Inge (4):
>   linux-user: Exit 0 when -h is used
>   linux-user: Add -help
>   linux-user: Add proper error messages for bad options
>   linux-user: Treat --foo options the same as -foo
> 
>  linux-user/main.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.1.1
> 
> 

Re: [Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Riku Voipio]

On Mon, Jul 13, 2015 at 01:08:02PM -0700, Meador Inge wrote:
> On Mon, Jul 06, 2015 at 09:43:20PM +0200, Laurent Vivier wrote:
> 
> > Global comment: you should use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h
 
> On second thought, I was following an existing pattern in 'main.c'. Really
> fixing this would require changing around 30 other locations too.  I think
> if we are going to switch to the EXIT_* macros, then the whole file should
> be changed in one patch and making that fix shouldn't hold this patch up.

I've just done a commit to for wholesale transition. This code is
only ever compiled for Linux, so portability doesn't matter. I think
it makes the code a bit more self-documenting.

Riku

Re: [Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Andreas Färber]

Am 28.03.2012 00:44, schrieb Meador Inge:
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  linux-user/main.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

/-F

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 1/4] linux-user: Exit 0 when -h is used

[Meador Inge]

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 linux-user/main.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 962677e..aabce83 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -51,7 +51,7 @@ int have_guest_base;
 unsigned long reserved_va;
 #endif
 
-static void usage(void);
+static void usage(int exitcode);
 
 static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
 const char *qemu_uname_release = CONFIG_UNAME_RELEASE;
@@ -2926,7 +2926,7 @@ void init_task_state(TaskState *ts)
 
 static void handle_arg_help(const char *arg)
 {
-    usage();
+    usage(0);
 }
 
 static void handle_arg_log(const char *arg)
@@ -2956,7 +2956,7 @@ static void handle_arg_set_env(const char *arg)
     r = p = strdup(arg);
     while ((token = strsep(&p, ",")) != NULL) {
         if (envlist_setenv(envlist, token) != 0) {
-            usage();
+            usage(1);
         }
     }
     free(r);
@@ -2968,7 +2968,7 @@ static void handle_arg_unset_env(const char *arg)
     r = p = strdup(arg);
     while ((token = strsep(&p, ",")) != NULL) {
         if (envlist_unsetenv(envlist, token) != 0) {
-            usage();
+            usage(1);
         }
     }
     free(r);
@@ -2984,7 +2984,7 @@ static void handle_arg_stack_size(const char *arg)
     char *p;
     guest_stack_size = strtoul(arg, &p, 0);
     if (guest_stack_size == 0) {
-        usage();
+        usage(1);
     }
 
     if (*p == 'M') {
@@ -3143,7 +3143,7 @@ struct qemu_argument arg_table[] = {
     {NULL, NULL, false, NULL, NULL, NULL}
 };
 
-static void usage(void)
+static void usage(int exitcode)
 {
     struct qemu_argument *arginfo;
     int maxarglen;
@@ -3204,7 +3204,7 @@ static void usage(void)
            "Note that if you provide several changes to a single variable\n"
            "the last change will stay in effect.\n");
 
-    exit(1);
+    exit(exitcode);
 }
 
 static int parse_args(int argc, char **argv)
@@ -3243,7 +3243,7 @@ static int parse_args(int argc, char **argv)
             if (!strcmp(r, arginfo->argv)) {
                 if (arginfo->has_arg) {
                     if (optind >= argc) {
-                        usage();
+                        usage(1);
                     }
                     arginfo->handle_opt(argv[optind]);
                     optind++;
@@ -3256,12 +3256,12 @@ static int parse_args(int argc, char **argv)
 
         /* no option matched the current argv */
         if (arginfo->handle_opt == NULL) {
-            usage();
+            usage(1);
         }
     }
 
     if (optind >= argc) {
-        usage();
+        usage(1);
     }
 
     filename = argv[optind];
-- 
1.7.7.6