[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 --]

Re: [Qemu-devel] Environment variables for user-mode QEMU

[Riku Voipio]

Bcc: 
Subject: Re: [Qemu-devel] Environment variables for user-mode QEMU
Reply-To: 
In-Reply-To: <87txmwoyqc.fsf@schwinge.name>
X-message-flag: Warning: message not sent with a DRM-Certified client 

On Wed, Apr 24, 2013 at 03:16:27PM +0200, Thomas Schwinge wrote:
> We have a need to pass environment variable assignments containing commas
> to user-mode QEMU.  The usage information currently says:
 
>     You can use -E and -U options or the QEMU_SET_ENV and
>     QEMU_UNSET_ENV environment variables to set and unset
>     environment variables for the target process.
>     It is possible to provide several variables by separating them
>     by commas in getsubopt(3) style. Additionally it is possible to
>     provide the -E and -U options multiple times.
 
>     $ env -i x86_64-linux-user/qemu-x86_64 -E x=y,y=z /usr/bin/printenv
>     y=z
>     x=y
> 
> Instead of this, we'd like to see:
> 
>     $ env -i x86_64-linux-user/qemu-x86_64 -E x=y,y=z /usr/bin/printenv
>     x=y,y=z
> 
> Due to the tokenization based on comma in
> linux-user/main.c:handle_arg_set_env, there is currently no way to
> achieve this -- other than pre-setting environment variables before
> running user-mode QEMU, which obviously isn't always desirable/possible
> (LD_LIBRARY_PATH, etc.).
> 
> Assuming there is a consensus, how would you like this implemented?
 
> Is it OK to change the semantics of -E (as well as -U, for symmetry?) to
> not handle multiple environment variable assignments (preliminary patch
> below), or does that functionality need to be preserved?  Something needs
> to be done about QEMU_SET_ENV and QEMU_UNSET_ENV then (if anyone is using
> these at all, which we can't disprove), as these are not very useful if
> they can handle only one environment variable.

We do not want to break backwards compatability. Also, adding an new
option to support passing commas would be ugly - and
we would have to support both options for a long time.

The only reason why we need the comma parsing is for the
QEMU_SET_ENV env variable, where the only way to set multiple env
variables is with commas.

However, it can be argued, that adding support for env variables 
in commit fc9c54124d134dbd76338a92a91804dab2df8166 broke passing commas
using the traditional "-E " parameters. 

So we could weasel ourself out of this mess by removing comma parsing when
setting env variables from "-E", while keep parsing when using QEMU_SET_ENV.

Riku

Re: [Qemu-devel] Environment variables for user-mode QEMU

[Thomas Schwinge]

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

Hi!

On Wed, 24 Apr 2013 15:16:27 +0200, I wrote:
> We have a need to pass environment variable assignments containing commas
> to user-mode QEMU.  The usage information currently says:
> 
>     You can use -E and -U options or the QEMU_SET_ENV and
>     QEMU_UNSET_ENV environment variables to set and unset
>     environment variables for the target process.
>     It is possible to provide several variables by separating them
>     by commas in getsubopt(3) style. Additionally it is possible to
>     provide the -E and -U options multiple times.
> 
>     $ env -i x86_64-linux-user/qemu-x86_64 -E x=y,y=z /usr/bin/printenv
>     y=z
>     x=y
> 
> Instead of this, we'd like to see:
> 
>     $ env -i x86_64-linux-user/qemu-x86_64 -E x=y,y=z /usr/bin/printenv
>     x=y,y=z
> 
> Due to the tokenization based on comma in
> linux-user/main.c:handle_arg_set_env, there is currently no way to
> achieve this -- other than pre-setting environment variables before
> running user-mode QEMU, which obviously isn't always desirable/possible
> (LD_LIBRARY_PATH, etc.).
> 
> Assuming there is a consensus, how would you like this implemented?
> 
> Is it OK to change the semantics of -E (as well as -U, for symmetry?) to
> not handle multiple environment variable assignments (preliminary patch
> below), or does that functionality need to be preserved?  Something needs
> to be done about QEMU_SET_ENV and QEMU_UNSET_ENV then (if anyone is using
> these at all, which we can't disprove), as these are not very useful if
> they can handle only one environment variable.

That is actually a "regression"/change introduced in commit
fc9c54124d134dbd76338a92a91804dab2df8166 (Johannes Schauer CCed, for your
information).  I'm attaching
0001-linux-user-Document-E-and-U-options.patch to update the
documentation for the linux-user QEMU.

The bsd-user QEMU has not been changed accordingly, by the way.


I'm attaching
0002-linux-user-Use-existing-envlist_parse_set-envlist_pa.patch for some
code simplifications.


> Or, should we perhaps have a new -env option that causes any later
> non-option arguments, that contain an equal sign, to be handled as
> environment variable assignments, just like the env command does?
> 
>     $ env -i x86_64-linux-user/qemu-x86_64 [some options] -env [more options] a=b,c d=e,f=a /usr/bin/printenv
>     a=b,c
>     d=e,f=a
> 
> I think I might prefer that solution.  As it is a new option, it also
> won't interfere with anyone's usage/expectations of the current behavior.

I'm attaching 0003-linux-user-New-env-option.patch for implementing that.


The patches have only been lightly tested; please review.


Grüße,
 Thomas



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-linux-user-Document-E-and-U-options.patch --]
[-- Type: text/x-diff, Size: 1534 bytes --]

From eaf53af8d75e001969a658d83d71dd48324c6bcb Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Wed, 24 Apr 2013 17:41:58 +0200
Subject: [PATCH 1/3] linux-user: Document -E and -U options.

Document changed behavior introducecd in commit
fc9c54124d134dbd76338a92a91804dab2df8166.

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 qemu-doc.texi |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git qemu-doc.texi qemu-doc.texi
index dfea4d3..3034af4 100644
--- qemu-doc.texi
+++ qemu-doc.texi
@@ -2683,13 +2683,11 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
 Set the x86 stack size in bytes (default=524288)
 @item -cpu model
 Select CPU model (-cpu help for list and additional feature selection)
-@item -ignore-environment
-Start with an empty environment. Without this option,
-the initial environment is a copy of the caller's environment.
-@item -E @var{var}=@var{value}
-Set environment @var{var} to @var{value}.
-@item -U @var{var}
-Remove @var{var} from the environment.
+@item -E @var{var}=@var{value}[,@var{var2}=@var{value2},...]
+Set environment @var{var} to @var{value}, @var{var2} to @var{value2},
+and so on.
+@item -U @var{var}[,var2,...]
+Remove @var{var}, @var{var2}, and so on from the environment.
 @item -B offset
 Offset guest address by the specified number of bytes.  This is useful when
 the address region required by guest applications is reserved on the host.
-- 
1.7.10.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-linux-user-Use-existing-envlist_parse_set-envlist_pa.patch --]
[-- Type: text/x-diff, Size: 1978 bytes --]

From 540f9fa2e9eb197bde3cb91403b6eaaa7d78ac4d Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Wed, 24 Apr 2013 17:28:02 +0200
Subject: [PATCH 2/3] linux-user: Use existing
 envlist_parse_set/envlist_parse_unset interface.

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 4e92a0b..7f81821 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 ebc06cf..4dbca28 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0003-linux-user-New-env-option.patch --]
[-- Type: text/x-diff, Size: 4442 bytes --]

From 71461a2caccfc31287e26ca1edf8ff0053a563f7 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Wed, 24 Apr 2013 17:59:17 +0200
Subject: [PATCH 3/3] linux-user: New -env option.

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 linux-user/main.c |   29 ++++++++++++++++++++++++++++-
 qemu-doc.texi     |    6 +++++-
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git linux-user/main.c linux-user/main.c
index 7f81821..7b38b54 100644
--- linux-user/main.c
+++ linux-user/main.c
@@ -38,6 +38,7 @@
 char *exec_path;
 
 int singlestep;
+int env_mode;
 const char *filename;
 const char *argv0;
 int gdbstub_port;
@@ -3202,6 +3203,11 @@ static void handle_arg_log_filename(const char *arg)
     qemu_set_log_filename(arg);
 }
 
+static void handle_arg_env(const char *arg)
+{
+    env_mode = 1;
+}
+
 static void handle_arg_set_env(const char *arg)
 {
     if (envlist_parse_set(envlist, arg) != 0) {
@@ -3354,6 +3360,8 @@ static const struct qemu_argument arg_table[] = {
      "size",       "set the stack size to 'size' bytes"},
     {"cpu",        "QEMU_CPU",         true,  handle_arg_cpu,
      "model",      "select CPU (-cpu help for list)"},
+    {"env",        "",                 false, handle_arg_env,
+     "",           "enable parsing of the ENV LIST (see below)"},
     {"E",          "QEMU_SET_ENV",     true,  handle_arg_set_env,
      "var=value",  "sets targets environment variable (see below)"},
     {"U",          "QEMU_UNSET_ENV",   true,  handle_arg_unset_env,
@@ -3390,7 +3398,7 @@ static void usage(void)
     int maxarglen;
     int maxenvlen;
 
-    printf("usage: qemu-" TARGET_ARCH " [options] program [arguments...]\n"
+    printf("usage: qemu-" TARGET_ARCH " [options] [env list] program [arguments...]\n"
            "Linux CPU emulator (compiled for " TARGET_ARCH " emulation)\n"
            "\n"
            "Options and associated environment variables:\n"
@@ -3444,9 +3452,11 @@ static void usage(void)
            "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"
+           "Only with -env it is possible to specify values containing commas.\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"
+           "    -env -U LD_PRELOAD,LD_DEBUG var1=val2 var2=val2\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");
@@ -3507,6 +3517,23 @@ static int parse_args(int argc, char **argv)
         }
     }
 
+    if (env_mode) {
+        for (;;) {
+            if (optind >= argc) {
+                break;
+            }
+            r = argv[optind];
+            if (strchr(r, '=') == NULL) {
+                break;
+            }
+            if (envlist_setenv(envlist, r) != 0) {
+                usage();
+            }
+
+            optind++;
+        }
+    }
+
     if (optind >= argc) {
         usage();
     }
diff --git qemu-doc.texi qemu-doc.texi
index 3034af4..f48a53e 100644
--- qemu-doc.texi
+++ qemu-doc.texi
@@ -2671,7 +2671,7 @@ qemu-i386 /usr/local/qemu-i386/wine/bin/wine \
 @subsection Command line options
 
 @example
-usage: qemu-i386 [-h] [-d] [-L path] [-s size] [-cpu model] [-g port] [-B offset] [-R size] program [arguments...]
+usage: qemu-i386 [-h] [-d] [-L path] [-s size] [-cpu model] [-g port] [-B offset] [-R size] [env list] program [arguments...]
 @end example
 
 @table @option
@@ -2683,6 +2683,10 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
 Set the x86 stack size in bytes (default=524288)
 @item -cpu model
 Select CPU model (-cpu help for list and additional feature selection)
+@item -env
+Enable parsing of the @var{env list}.  This means that any
+@var{var}=@var{value} assignments before @var{program} will be added
+to the environment.
 @item -E @var{var}=@var{value}[,@var{var2}=@var{value2},...]
 Set environment @var{var} to @var{value}, @var{var2} to @var{value2},
 and so on.
-- 
1.7.10.4


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

[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

[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 +++++++++++++++++++++++++++++++++++----------------
 qemu-doc.texi     |    3 ---
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git linux-user/main.c linux-user/main.c
index 50bbadf..c539125 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);
 }
diff --git qemu-doc.texi qemu-doc.texi
index dfea4d3..64493eb 100644
--- qemu-doc.texi
+++ qemu-doc.texi
@@ -2683,9 +2683,6 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
 Set the x86 stack size in bytes (default=524288)
 @item -cpu model
 Select CPU model (-cpu help for list and additional feature selection)
-@item -ignore-environment
-Start with an empty environment. Without this option,
-the initial environment is a copy of the caller's environment.
 @item -E @var{var}=@var{value}
 Set environment @var{var} to @var{value}.
 @item -U @var{var}
-- 
1.7.10.4

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

[Peter Maydell]

On 25 April 2013 16:06, 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.
>
> --- qemu-doc.texi
> +++ qemu-doc.texi
> @@ -2683,9 +2683,6 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
>  Set the x86 stack size in bytes (default=524288)
>  @item -cpu model
>  Select CPU model (-cpu help for list and additional feature selection)
> -@item -ignore-environment
> -Start with an empty environment. Without this option,
> -the initial environment is a copy of the caller's environment.
>  @item -E @var{var}=@var{value}
>  Set environment @var{var} to @var{value}.
>  @item -U @var{var}

This appears to be a stray unrelated change?

-- PMM

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

[Thomas Schwinge]

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

Hi!

On Thu, 25 Apr 2013 16:52:28 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 April 2013 16:06, 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.
> >
> > --- qemu-doc.texi
> > +++ qemu-doc.texi
> > @@ -2683,9 +2683,6 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
> >  Set the x86 stack size in bytes (default=524288)
> >  @item -cpu model
> >  Select CPU model (-cpu help for list and additional feature selection)
> > -@item -ignore-environment
> > -Start with an empty environment. Without this option,
> > -the initial environment is a copy of the caller's environment.
> >  @item -E @var{var}=@var{value}
> >  Set environment @var{var} to @var{value}.
> >  @item -U @var{var}
> 
> This appears to be a stray unrelated change?

Well, this is also a follow-up to the commit
fc9c54124d134dbd76338a92a91804dab2df8166 (referenced in the commit
message above), which silently removed that option...


Grüße,
 Thomas

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

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

[Peter Maydell]

On 25 April 2013 17:18, Thomas Schwinge <thomas@codesourcery.com> wrote:
> On Thu, 25 Apr 2013 16:52:28 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 April 2013 16:06, Thomas Schwinge <thomas@codesourcery.com> wrote:
>> > --- qemu-doc.texi
>> > +++ qemu-doc.texi
>> > @@ -2683,9 +2683,6 @@ Set the x86 elf interpreter prefix (default=/usr/local/qemu-i386)
>> >  Set the x86 stack size in bytes (default=524288)
>> >  @item -cpu model
>> >  Select CPU model (-cpu help for list and additional feature selection)
>> > -@item -ignore-environment
>> > -Start with an empty environment. Without this option,
>> > -the initial environment is a copy of the caller's environment.
>> >  @item -E @var{var}=@var{value}
>> >  Set environment @var{var} to @var{value}.
>> >  @item -U @var{var}
>>
>> This appears to be a stray unrelated change?
>
> Well, this is also a follow-up to the commit
> fc9c54124d134dbd76338a92a91804dab2df8166 (referenced in the commit
> message above), which silently removed that option...

It's still a separate change, so it should go in its own
patch, please.

thanks
-- PMM