[Qemu-devel] [PATCH, resend] linux-user: environment variables

[Qemu-devel] [PATCH, resend] linux-user: environment variables

[Thomas Schwinge]

Hi!

I'm resending here the patches previously posted and described in the thread
starting at
<http://news.gmane.org/find-root.php?message_id=%3C87txmwoyqc.fsf%40schwinge.name%3E>.
In short, fix a bug in util/envlist, code simplification, and then restore the
original behavior of the -E and -U command-line options.


Grüße,
 Thomas

[Qemu-devel] [PATCH 1/4] util/envlist: Properly forward a callback's error in envlist_parse.

[Thomas Schwinge]

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 util/envlist.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git util/envlist.c util/envlist.c
index ebc06cf..cbbf7e5 100644
--- util/envlist.c
+++ util/envlist.c
@@ -109,9 +109,10 @@ envlist_parse(envlist_t *envlist, const char *env,
 
 	envvar = strtok_r(tmpenv, ",", &envsave);
 	while (envvar != NULL) {
-		if ((*callback)(envlist, envvar) != 0) {
+		int err;
+		if ((err = (*callback)(envlist, envvar)) != 0) {
 			free(tmpenv);
-			return (errno);
+			return (err);
 		}
 		envvar = strtok_r(NULL, ",", &envsave);
 	}
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] util/envlist: Properly forward a callback's error in envlist_parse.

[Peter Maydell]

On 29 May 2013 14:50, Thomas Schwinge <thomas@codesourcery.com> wrote:
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> ---
>  util/envlist.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git util/envlist.c util/envlist.c
> index ebc06cf..cbbf7e5 100644
> --- util/envlist.c
> +++ util/envlist.c

NB: your process for generating patch mails seems to be slightly
wrong -- the diff header in a patch mail should look like this:
diff --git a/util/envlist.c b/util/envlist.c
index ebc06cf..cbbf7e5 100644
--- a/util/envlist.c
+++ b/util/envlist.c

(note the extra 'a' and 'b'); otherwise 'git am patch.mbox'
will complain when you try to apply the patch.

thanks
-- PMM

[Qemu-devel] [PATCH 2/4] linux-user: Use existing envlist_parse_set/envlist_parse_unset interface.

[Thomas Schwinge]

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 linux-user/main.c |   18 ++++--------------
 util/envlist.c    |    4 ++--
 2 files changed, 6 insertions(+), 16 deletions(-)

diff --git linux-user/main.c linux-user/main.c
index b97b8cf..a0ea161 100644
--- linux-user/main.c
+++ linux-user/main.c
@@ -3204,26 +3204,16 @@ static void handle_arg_log_filename(const char *arg)
 
 static void handle_arg_set_env(const char *arg)
 {
-    char *r, *p, *token;
-    r = p = strdup(arg);
-    while ((token = strsep(&p, ",")) != NULL) {
-        if (envlist_setenv(envlist, token) != 0) {
-            usage();
-        }
+    if (envlist_parse_set(envlist, arg) != 0) {
+        usage();
     }
-    free(r);
 }
 
 static void handle_arg_unset_env(const char *arg)
 {
-    char *r, *p, *token;
-    r = p = strdup(arg);
-    while ((token = strsep(&p, ",")) != NULL) {
-        if (envlist_unsetenv(envlist, token) != 0) {
-            usage();
-        }
+    if (envlist_parse_unset(envlist, arg) != 0) {
+        usage();
     }
-    free(r);
 }
 
 static void handle_arg_argv0(const char *arg)
diff --git util/envlist.c util/envlist.c
index cbbf7e5..8027bbf 100644
--- util/envlist.c
+++ util/envlist.c
@@ -55,10 +55,10 @@ envlist_free(envlist_t *envlist)
 
 /*
  * Parses comma separated list of set/modify environment
- * variable entries and updates given enlist accordingly.
+ * variable entries and updates given envlist accordingly.
  *
  * For example:
- *     envlist_parse(el, "HOME=foo,SHELL=/bin/sh");
+ *     envlist_parse_set(el, "HOME=foo,SHELL=/bin/sh");
  *
  * inserts/sets environment variables HOME and SHELL.
  *
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/4] linux-user: Use existing envlist_parse_set/envlist_parse_unset interface.

[Peter Maydell]

On 29 May 2013 14:50, Thomas Schwinge <thomas@codesourcery.com> wrote:
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
> ---
>  linux-user/main.c |   18 ++++--------------
>  util/envlist.c    |    4 ++--
>  2 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git linux-user/main.c linux-user/main.c
> index b97b8cf..a0ea161 100644
> --- linux-user/main.c
> +++ linux-user/main.c
> @@ -3204,26 +3204,16 @@ static void handle_arg_log_filename(const char *arg)
>
>  static void handle_arg_set_env(const char *arg)
>  {
> -    char *r, *p, *token;
> -    r = p = strdup(arg);
> -    while ((token = strsep(&p, ",")) != NULL) {
> -        if (envlist_setenv(envlist, token) != 0) {
> -            usage();
> -        }
> +    if (envlist_parse_set(envlist, arg) != 0) {
> +        usage();
>      }
> -    free(r);
>  }
>
>  static void handle_arg_unset_env(const char *arg)
>  {
> -    char *r, *p, *token;
> -    r = p = strdup(arg);
> -    while ((token = strsep(&p, ",")) != NULL) {
> -        if (envlist_unsetenv(envlist, token) != 0) {
> -            usage();
> -        }
> +    if (envlist_parse_unset(envlist, arg) != 0) {
> +        usage();
>      }
> -    free(r);
>  }

This looks OK...

>
>  static void handle_arg_argv0(const char *arg)
> diff --git util/envlist.c util/envlist.c
> index cbbf7e5..8027bbf 100644
> --- util/envlist.c
> +++ util/envlist.c
> @@ -55,10 +55,10 @@ envlist_free(envlist_t *envlist)
>
>  /*
>   * Parses comma separated list of set/modify environment
> - * variable entries and updates given enlist accordingly.
> + * variable entries and updates given envlist accordingly.
>   *
>   * For example:
> - *     envlist_parse(el, "HOME=foo,SHELL=/bin/sh");
> + *     envlist_parse_set(el, "HOME=foo,SHELL=/bin/sh");
>   *
>   * inserts/sets environment variables HOME and SHELL.
>   *

...but this bit needs to be a separate patch.

-- PMM

[Qemu-devel] [PATCH 3/4] linux-user: Tell handler_arg_* which context they're invoked from.

[Thomas Schwinge]

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 linux-user/main.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git linux-user/main.c linux-user/main.c
index a0ea161..b7d49f4 100644
--- linux-user/main.c
+++ linux-user/main.c
@@ -3180,12 +3180,17 @@ void init_task_state(TaskState *ts)
     ts->sigqueue_table[i].next = NULL;
 }
 
-static void handle_arg_help(const char *arg)
+typedef enum {
+    ARG_ORIGIN_ENV,
+    ARG_ORIGIN_CMDLINE
+} arg_origin;
+
+static void handle_arg_help(arg_origin whence, const char *arg)
 {
     usage();
 }
 
-static void handle_arg_log(const char *arg)
+static void handle_arg_log(arg_origin whence, const char *arg)
 {
     int mask;
 
@@ -3197,31 +3202,31 @@ static void handle_arg_log(const char *arg)
     qemu_set_log(mask);
 }
 
-static void handle_arg_log_filename(const char *arg)
+static void handle_arg_log_filename(arg_origin whence, const char *arg)
 {
     qemu_set_log_filename(arg);
 }
 
-static void handle_arg_set_env(const char *arg)
+static void handle_arg_set_env(arg_origin whence, const char *arg)
 {
     if (envlist_parse_set(envlist, arg) != 0) {
         usage();
     }
 }
 
-static void handle_arg_unset_env(const char *arg)
+static void handle_arg_unset_env(arg_origin whence, const char *arg)
 {
     if (envlist_parse_unset(envlist, arg) != 0) {
         usage();
     }
 }
 
-static void handle_arg_argv0(const char *arg)
+static void handle_arg_argv0(arg_origin whence, const char *arg)
 {
     argv0 = strdup(arg);
 }
 
-static void handle_arg_stack_size(const char *arg)
+static void handle_arg_stack_size(arg_origin whence, const char *arg)
 {
     char *p;
     guest_stack_size = strtoul(arg, &p, 0);
@@ -3236,12 +3241,12 @@ static void handle_arg_stack_size(const char *arg)
     }
 }
 
-static void handle_arg_ld_prefix(const char *arg)
+static void handle_arg_ld_prefix(arg_origin whence, const char *arg)
 {
     interp_prefix = strdup(arg);
 }
 
-static void handle_arg_pagesize(const char *arg)
+static void handle_arg_pagesize(arg_origin whence, const char *arg)
 {
     qemu_host_page_size = atoi(arg);
     if (qemu_host_page_size == 0 ||
@@ -3251,17 +3256,17 @@ static void handle_arg_pagesize(const char *arg)
     }
 }
 
-static void handle_arg_gdb(const char *arg)
+static void handle_arg_gdb(arg_origin whence, const char *arg)
 {
     gdbstub_port = atoi(arg);
 }
 
-static void handle_arg_uname(const char *arg)
+static void handle_arg_uname(arg_origin whence, const char *arg)
 {
     qemu_uname_release = strdup(arg);
 }
 
-static void handle_arg_cpu(const char *arg)
+static void handle_arg_cpu(arg_origin whence, const char *arg)
 {
     cpu_model = strdup(arg);
     if (cpu_model == NULL || is_help_option(cpu_model)) {
@@ -3274,13 +3279,13 @@ static void handle_arg_cpu(const char *arg)
 }
 
 #if defined(CONFIG_USE_GUEST_BASE)
-static void handle_arg_guest_base(const char *arg)
+static void handle_arg_guest_base(arg_origin whence, const char *arg)
 {
     guest_base = strtol(arg, NULL, 0);
     have_guest_base = 1;
 }
 
-static void handle_arg_reserved_va(const char *arg)
+static void handle_arg_reserved_va(arg_origin whence, const char *arg)
 {
     char *p;
     int shift = 0;
@@ -3317,17 +3322,17 @@ static void handle_arg_reserved_va(const char *arg)
 }
 #endif
 
-static void handle_arg_singlestep(const char *arg)
+static void handle_arg_singlestep(arg_origin whence, const char *arg)
 {
     singlestep = 1;
 }
 
-static void handle_arg_strace(const char *arg)
+static void handle_arg_strace(arg_origin whence, const char *arg)
 {
     do_strace = 1;
 }
 
-static void handle_arg_version(const char *arg)
+static void handle_arg_version(arg_origin whence, const char *arg)
 {
     printf("qemu-" TARGET_ARCH " version " QEMU_VERSION QEMU_PKGVERSION
            ", Copyright (c) 2003-2008 Fabrice Bellard\n");
@@ -3338,7 +3343,7 @@ struct qemu_argument {
     const char *argv;
     const char *env;
     bool has_arg;
-    void (*handle_opt)(const char *arg);
+    void (*handle_opt)(arg_origin whence, const char *arg);
     const char *example;
     const char *help;
 };
@@ -3467,7 +3472,7 @@ static int parse_args(int argc, char **argv)
 
         r = getenv(arginfo->env);
         if (r != NULL) {
-            arginfo->handle_opt(r);
+            arginfo->handle_opt(ARG_ORIGIN_ENV, r);
         }
     }
 
@@ -3492,10 +3497,10 @@ static int parse_args(int argc, char **argv)
                     if (optind >= argc) {
                         usage();
                     }
-                    arginfo->handle_opt(argv[optind]);
+                    arginfo->handle_opt(ARG_ORIGIN_CMDLINE, argv[optind]);
                     optind++;
                 } else {
-                    arginfo->handle_opt(NULL);
+                    arginfo->handle_opt(ARG_ORIGIN_CMDLINE, NULL);
                 }
                 break;
             }
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] linux-user: Restore original behavior of the -E and -U command-line options.

[Thomas Schwinge]

Revert the change in behavior that had been introducecd in commit
fc9c54124d134dbd76338a92a91804dab2df8166 for the -E and -U command-line
options, but keep the comma-splitting for the QEMU_SET_ENV and QEMU_UNSET_ENV
environment variables.

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 linux-user/main.c |   51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git linux-user/main.c linux-user/main.c
index b7d49f4..874791b 100644
--- linux-user/main.c
+++ linux-user/main.c
@@ -3209,15 +3209,37 @@ static void handle_arg_log_filename(arg_origin whence, const char *arg)
 
 static void handle_arg_set_env(arg_origin whence, const char *arg)
 {
-    if (envlist_parse_set(envlist, arg) != 0) {
-        usage();
+    switch (whence) {
+    case ARG_ORIGIN_ENV:
+        if (envlist_parse_set(envlist, arg) != 0) {
+            usage();
+        }
+        break;
+    case ARG_ORIGIN_CMDLINE:
+        if (envlist_setenv(envlist, arg) != 0) {
+            usage();
+        }
+        break;
+    default:
+        abort();
     }
 }
 
 static void handle_arg_unset_env(arg_origin whence, const char *arg)
 {
-    if (envlist_parse_unset(envlist, arg) != 0) {
-        usage();
+    switch (whence) {
+    case ARG_ORIGIN_ENV:
+        if (envlist_parse_unset(envlist, arg) != 0) {
+            usage();
+        }
+        break;
+    case ARG_ORIGIN_CMDLINE:
+        if (envlist_unsetenv(envlist, arg) != 0) {
+            usage();
+        }
+        break;
+    default:
+        abort();
     }
 }
 
@@ -3443,18 +3465,15 @@ static void usage(void)
            guest_stack_size);
 
     printf("\n"
-           "You can use -E and -U options or the QEMU_SET_ENV and\n"
-           "QEMU_UNSET_ENV environment variables to set and unset\n"
-           "environment variables for the target process.\n"
-           "It is possible to provide several variables by separating them\n"
-           "by commas in getsubopt(3) style. Additionally it is possible to\n"
-           "provide the -E and -U options multiple times.\n"
-           "The following lines are equivalent:\n"
-           "    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
-           "    -E var1=val2,var2=val2 -U LD_PRELOAD,LD_DEBUG\n"
-           "    QEMU_SET_ENV=var1=val2,var2=val2 QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
-           "Note that if you provide several changes to a single variable\n"
-           "the last change will stay in effect.\n");
+"The -E and -U command-line options can be provided multiple times to set\n"
+"and unset environment variables for the target process, and -E can be used\n"
+"to specify values containing commas.  When using the QEMU_SET_ENV and\n"
+"QEMU_UNSET_ENV environment variables, a comma is used in getsubopt(3) style\n"
+"to set or unset several variables.  The following lines are equivalent:\n"
+"    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
+"    QEMU_SET_ENV=var1=val2,var2=val2 QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
+"Note that if you provide several changes to a single variable, the last\n"
+"change will stay in effect.\n");
 
     exit(1);
 }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] linux-user: Restore original behavior of the -E and -U command-line options.

[Alexander Graf]

On 29.05.2013, at 15:50, Thomas Schwinge wrote:

> Revert the change in behavior that had been introducecd in commit
> fc9c54124d134dbd76338a92a91804dab2df8166 for the -E and -U command-line
> options, but keep the comma-splitting for the QEMU_SET_ENV and QEMU_UNSET_ENV
> environment variables.
> 
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
> ---
> linux-user/main.c |   51 +++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git linux-user/main.c linux-user/main.c
> index b7d49f4..874791b 100644
> --- linux-user/main.c
> +++ linux-user/main.c
> @@ -3209,15 +3209,37 @@ static void handle_arg_log_filename(arg_origin whence, const char *arg)
> 
> static void handle_arg_set_env(arg_origin whence, const char *arg)
> {
> -    if (envlist_parse_set(envlist, arg) != 0) {
> -        usage();
> +    switch (whence) {
> +    case ARG_ORIGIN_ENV:
> +        if (envlist_parse_set(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    case ARG_ORIGIN_CMDLINE:
> +        if (envlist_setenv(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    default:
> +        abort();
>     }
> }
> 
> static void handle_arg_unset_env(arg_origin whence, const char *arg)
> {
> -    if (envlist_parse_unset(envlist, arg) != 0) {
> -        usage();
> +    switch (whence) {
> +    case ARG_ORIGIN_ENV:
> +        if (envlist_parse_unset(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    case ARG_ORIGIN_CMDLINE:
> +        if (envlist_unsetenv(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    default:
> +        abort();
>     }
> }
> 
> @@ -3443,18 +3465,15 @@ static void usage(void)
>            guest_stack_size);
> 
>     printf("\n"
> -           "You can use -E and -U options or the QEMU_SET_ENV and\n"
> -           "QEMU_UNSET_ENV environment variables to set and unset\n"
> -           "environment variables for the target process.\n"
> -           "It is possible to provide several variables by separating them\n"
> -           "by commas in getsubopt(3) style. Additionally it is possible to\n"
> -           "provide the -E and -U options multiple times.\n"
> -           "The following lines are equivalent:\n"
> -           "    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
> -           "    -E var1=val2,var2=val2 -U LD_PRELOAD,LD_DEBUG\n"
> -           "    QEMU_SET_ENV=var1=val2,var2=val2 QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
> -           "Note that if you provide several changes to a single variable\n"
> -           "the last change will stay in effect.\n");
> +"The -E and -U command-line options can be provided multiple times to set\n"
> +"and unset environment variables for the target process, and -E can be used\n"
> +"to specify values containing commas.  When using the QEMU_SET_ENV and\n"
> +"QEMU_UNSET_ENV environment variables, a comma is used in getsubopt(3) style\n"
> +"to set or unset several variables.  The following lines are equivalent:\n"
> +"    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
> +"    QEMU_SET_ENV=var1=val2,var2=val2 QEMU_UNSET_ENV=LD_PRELOAD,LD_DEBUG\n"
> +"Note that if you provide several changes to a single variable, the last\n"
> +"change will stay in effect.\n");

I suppose the additional indent was on purpose. It would also be nice to document the availability of ,, here now - I doubt everyone knows that it's possible to use it here.

Otherwise the patch set looks fine to me.


Alex

Re: [Qemu-devel] [PATCH 4/4] linux-user: Restore original behavior of the -E and -U command-line options.

[Peter Maydell]

On 29 May 2013 14:50, Thomas Schwinge <thomas@codesourcery.com> wrote:
> Revert the change in behavior that had been introducecd in commit
> fc9c54124d134dbd76338a92a91804dab2df8166 for the -E and -U command-line
> options, but keep the comma-splitting for the QEMU_SET_ENV and QEMU_UNSET_ENV
> environment variables.
>
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
> ---
>  linux-user/main.c |   51 +++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git linux-user/main.c linux-user/main.c
> index b7d49f4..874791b 100644
> --- linux-user/main.c
> +++ linux-user/main.c
> @@ -3209,15 +3209,37 @@ static void handle_arg_log_filename(arg_origin whence, const char *arg)
>
>  static void handle_arg_set_env(arg_origin whence, const char *arg)
>  {
> -    if (envlist_parse_set(envlist, arg) != 0) {
> -        usage();
> +    switch (whence) {
> +    case ARG_ORIGIN_ENV:
> +        if (envlist_parse_set(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    case ARG_ORIGIN_CMDLINE:
> +        if (envlist_setenv(envlist, arg) != 0) {
> +            usage();
> +        }
> +        break;
> +    default:
> +        abort();
>      }
>  }

I agree with Alex's comments; also this function could use a brief
comment explaining why we treat env and command line differently,
as a guard against somebody at a future date undoing this change
in the name of simplification and conistency.

thanks
-- PMM

Re: [Qemu-devel] [PATCH, resend] linux-user: environment variables

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

Hi!

On Wed, 29 May 2013 15:50:30 +0200, I wrote:
> I'm resending here the patches previously posted and described in the thread
> starting at
> <http://news.gmane.org/find-root.php?message_id=%3C87txmwoyqc.fsf%40schwinge.name%3E>.
> In short, fix a bug in util/envlist, code simplification, and then restore the
> original behavior of the -E and -U command-line options.

Ping.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]