[m-rev.] for review: MERCURY_OPTIONS-progname

[Paul Bone]

On Tue, May 20, 2008 at 05:11:06PM +1000, Zoltan Somogyi wrote:
> MERCURY_OPTIONS is a crude way to give options to Mercury programs, since it
> applies to all Mercury programs, even though we often want to give options to
> only one specific Mercury program. (For example, we may want to give it only
> to a test program's executable, and not to the invocation of the compiler
> that generates it.) This diff makes the runtime system, when executing a
> program named "progname", look for and process the environment variable
> whose name is "MERCURY_OPTIONS-progname". This allows options to be given
> at runtime to only one specific program.
> 
> runtime/mercury_wrapper.c:
> 	Make the change described above.
> 
> runtime/mercury_wrapper.h:
> 	Fix some obsolete documentation, and add some up-to-date documentation.
> 
> doc/user_guide.texi:
> 	Document the change.
> 
> util/mkinit.c:
> 	Fix indentation.
> 
> Zoltan.
>
> Index: runtime/mercury_wrapper.c
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_wrapper.c,v
> retrieving revision 1.188
> diff -u -b -r1.188 mercury_wrapper.c
> --- runtime/mercury_wrapper.c	18 Mar 2008 03:09:43 -0000	1.188
> +++ runtime/mercury_wrapper.c	20 May 2008 07:07:46 -0000
> @@ -987,28 +987,67 @@
>  }
>  
>  /*
> -** MR_process_environment_options() is a function to parse the MERCURY_OPTIONS
> -** environment variable.  
> +** MR_process_environment_options() is a function to parse the options put
> +** into MR_runtime_flags by mkinit, the MERCURY_OPTIONS environment variable,
> +** and the MERCURY_OPTIONS-progname environment variable.
>  */ 
>  
> +#define MERCURY_OPTIONS     "MERCURY_OPTIONS"
> +
>  static void
>  MR_process_environment_options(void)
>  {
> -    char    *env_options;
> +    char        *gen_env_options;
> +    char        *prog_env_options;
> +    char        *prog_env_option_name;
> +    int         prog_env_option_name_len;
> +    int         mercury_options_len;
> +    const char  *progname;
> +    const char  *s;
> +
> +    gen_env_options = getenv(MERCURY_OPTIONS);
> +    if (gen_env_options == NULL) {
> +        gen_env_options = (char *) "";
> +    }

Why use a cast here?

I beleive that "" is normaly const char *.  Can gen_env_options be made
const char *?

>  
> -    env_options = getenv("MERCURY_OPTIONS");
> -    if (env_options == NULL) {
> -        env_options = (char *) "";
> +    /* Find out the program's name, stripping off any directory names. */
> +    progname = MR_progname;
> +    for (s = progname; *s != '\0'; s++) {
> +        if (*s == '/') {
> +            progname = s + 1;
>      }
> +    }
> +
> +    /* Build the program-specific option's name: MERCURY_OPTIONS-progname. */
> +    mercury_options_len = strlen(MERCURY_OPTIONS);
> +    prog_env_option_name_len = mercury_options_len + 1 + strlen(progname);
> +    prog_env_option_name = MR_GC_NEW_ARRAY(char, prog_env_option_name_len);
> +    strcpy(prog_env_option_name, MERCURY_OPTIONS);
> +    prog_env_option_name[mercury_options_len] = '-';
> +    strcpy(prog_env_option_name + mercury_options_len + 1, progname);
>  
> -    if (env_options[0] != '\0' || MR_runtime_flags[0] != '\0') {
> -        const char  *cmd;
> -        char        *arg_str, **argv;
> +    prog_env_options = getenv(prog_env_option_name);
> +    if (prog_env_options == NULL) {
> +        prog_env_options = (char *) "";
> +    }

And here.

> @@ -1016,28 +1055,92 @@
>          ** at the start of the options before passing them to MR_make_argv()
>          ** and then to getopt().
>          */
> -        cmd = "mercury_runtime ";
> -        cmd_len = strlen(cmd);
> +
> +        dummy_cmd = "mercury_runtime";
> +        dummy_cmd_len = strlen(dummy_cmd);
>          runtime_flags_len = strlen(MR_runtime_flags);
> -        dummy_command_line = MR_GC_NEW_ARRAY(char,
> -            cmd_len + runtime_flags_len + 1 + strlen(env_options) + 1);
> -        strcpy(dummy_command_line, cmd);
> -        strcpy(dummy_command_line + cmd_len, MR_runtime_flags);
> -        dummy_command_line[cmd_len + runtime_flags_len] = ' ';
> -        strcpy(dummy_command_line + cmd_len + runtime_flags_len + 1,
> -            env_options);
> +        gen_env_options_len = strlen(gen_env_options);
> +        prog_env_options_len = strlen(prog_env_options);
> +        dummy_command_line_len =
> +            dummy_cmd_len + 1 +
> +            runtime_flags_len + 1 +
> +            gen_env_options_len + 1 +
> +            prog_env_options_len + 1;
> +        dummy_command_line = MR_GC_NEW_ARRAY(char, dummy_command_line_len);
> +
> +        next_slot = 0;
> +
> +        strcpy(dummy_command_line + next_slot, dummy_cmd);
> +        next_slot += dummy_cmd_len;
> +        dummy_command_line[next_slot] = ' ';
> +        next_slot += 1;
> +        strcpy(dummy_command_line + next_slot, MR_runtime_flags);
> +        next_slot += runtime_flags_len;
> +        dummy_command_line[next_slot] = ' ';
> +        next_slot += 1;
> +        strcpy(dummy_command_line + next_slot, gen_env_options);
> +        next_slot += gen_env_options_len;
> +        dummy_command_line[next_slot] = ' ';
> +        next_slot += 1;
> +        strcpy(dummy_command_line + next_slot, prog_env_options);
> +        next_slot += prog_env_options_len;
> +        dummy_command_line[next_slot] = '\0';
> +        next_slot += 1;
> +
> +        /* Sanity check. */
> +        if (next_slot != dummy_command_line_len) {
> +            MR_fatal_error("next_slot != dummy_command_line_len");
> +        }
> +
> +#ifdef MR_DEBUG_ARGUMENT_HANDLING
> +        /* Enable this is if you need to debug this code. */
> +        printf("progname = <%s>\n", progname);
> +        printf("MR_runtime_flags = <%s>\n", MR_runtime_flags);
> +        printf("gen_env_options = <%s>\n", gen_env_options);
> +        printf("prog_env_options = <%s>\n", prog_env_options);
> +        printf("dummy_command_line = <%s>\n", dummy_command_line);
> +#endif
>          
> -        error_msg = MR_make_argv(dummy_command_line, &arg_str, &argv, &argc);
> +        error_msg = MR_make_argv(dummy_command_line, &option_arg_str,
> +            &option_argv, &option_argc);
>          if (error_msg != NULL) {
> -            MR_fatal_error("error parsing the MERCURY_OPTIONS "
> -                "environment variable:\n%s\n", error_msg);
> +            char    *where_buf;
> +            int     where_buf_next;
> +
> +            where_buf = MR_GC_NEW_ARRAY(char, 1000);
> +            where_buf[0] = '\0';
> +            where_buf_next = strlen(where_buf);
> +
> +            if (gen_env_options[0] != '\0') {
> +                sprintf(where_buf + where_buf_next,
> +                    "%sthe %s environment variable",
> +                    where_buf_next == 0 ? "" : " and/or ",
> +                    MERCURY_OPTIONS);
> +                where_buf_next = strlen(where_buf);

This could crash when supplied with a large-enough invalid command line
option.  snprintf will not overflow the buffer.

>          }
> -        MR_GC_free(dummy_command_line);
>  
> -        MR_process_options(argc, argv);
> +            if (prog_env_options[0] != '\0') {
> +                sprintf(where_buf + where_buf_next,
> +                    "%sthe %s environment variable",
> +                    where_buf_next == 0 ? "" : " and/or ",
> +                    prog_env_option_name);
> +                where_buf_next = strlen(where_buf);
> +            }
>  
> -        MR_GC_free(arg_str);
> -        MR_GC_free(argv);
> +            if (MR_runtime_flags[0] != '\0') {
> +                sprintf(where_buf + where_buf_next,
> +                    "%sthe runtime options built into the executable",
> +                    where_buf_next == 0 ? "" : " and/or ");
> +                where_buf_next = strlen(where_buf);
> +            }
> +
> +            MR_fatal_error("error parsing %s:\n%s\n", where_buf, error_msg);
> +        }
> +
> +        MR_GC_free(dummy_command_line);
> +        MR_process_options(option_argc, option_argv);
> +        MR_GC_free(option_arg_str);
> +        MR_GC_free(option_argv);
>      }
>  }
>  

Everything else looks good to me.


--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------