Re: [PATCH 2/3] objdump: introduce OBJDUMP_COLORS environment variable

[Nick Clifton <nickc@redhat.com>]

Hi Andrew,

> Add OBJDUMP_COLORS environment variable, and allow this to control the
> default behaviour of objdump when --disassembler-color is not used,
> and we are disassembling to a terminal.

Whilst this is a nice idea, there is a problem with using environment
variables: users often fail to include them when reporting a bug.

Imagine for example a bug report about the colored disassembly being
broken somehow and the user has forgotten to mention that they have
OBJDUMP_COLORS set to "extended".  We could waste hours trying to
track down the problem only to find that we had been looking at the
wrong piece of code.

I cannot think of a nice way to resolve this problem.  One idea that
does spring to mind is to have objdump generate a message to stdout
when the environment variable is used.  That way there is a good
likelihood that the message will be included in a bug report.  But
this does mean new, extraneous output will now appear in the
disassembly output.



> +static enum color_selection
> +objdump_default_disassembler_color_mode (void)
> +{
> +  enum color_selection mode;
> +
> +  if (isatty (1))
> +    {
> +      const char *tmp = getenv (objdump_colors_var);
> +
> +      if (tmp == NULL || strncmp (tmp, "color", 5) == 0)

Why strncmp() rather than strcmp () ?  [Because of patch 3/3 I expect...]

> +	mode = on;
> +      if (strncmp (tmp, "extended", 8) == 0)
> +	mode = extended;
> +      else if (strncmp (tmp, "off", 3) == 0)
> +	mode = off;
> +      else
> +	mode = on;

It would probably be best to check for strncmp (tmp, "on") here
and report if there is no match - that way users will know that
they have a malformed OBJDUMP_COLOR environment variable.

Cheers
   Nick