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

login
register
mail settings
Submitter Thomas Schwinge
Date May 29, 2013, 1:50 p.m.
Message ID <1369835434-27727-5-git-send-email-thomas@codesourcery.com>
Download mbox | patch
Permalink /patch/247396/
State New
Headers show

Comments

Thomas Schwinge - May 29, 2013, 1:50 p.m.
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(-)
Alexander Graf - June 14, 2013, 1:08 a.m.
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
Peter Maydell - June 27, 2013, 5:54 p.m.
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

Patch

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);
 }