Re: [PATCH v2] Enable 'make CONFIG_FOO=y oldconfig'

[David Woodhouse <dwmw2@infradead.org>]

On Fri, 2011-07-29 at 21:15 -0400, Arnaud Lacombe wrote:
> Technically, kconfig already provides you all the interfaces to set
> symbol to a given value, for `randconfig' via "rand.config", for
> `all{yes,mod,no}config', via all.{yes,mod,no}config, for oldconfig via
> .config. Why another interface ?

It's useful for some people to be able to do this as simply as possible
from the command line, and they get very upset if you suggest that they
might use those other methods instead.

In particular, some people currently use the legacy 'ARCH=i386' and
'ARCH=x86_64' settings on the command line in order to switch the value
of CONFIG_64BIT on and off for various *config targets¹. In fact, I
think that's the *only* thing that those legacy ARCH settings still do;
they could (technically, at least) be killed off if we have an
alternative way of doing it. This provides an alternative, more generic
way of doing it for *all* config options; not just CONFIG_64BIT and not
just for x86.

(¹ Note: in fact, *many* people use 'ARCH=i386' and 'ARCH=x86_64', but
not really for this purpose. Most people use it just to work around the
bug I fixed in my other patch — that the value of CONFIG_64BIT is
*always* overridden to match the build host, unless you do *something*
on the command line to work around it. Just ARCH=x86 would suffice,
since that makes CONFIG_64BIT work sanely again.)

> > +void conf_set_symbol_from_env(char *str)
> > +{
> > +       char *p = strchr(str, '=');
> > +       struct symbol *sym;
> > +       int def = S_DEF_USER;
> > +       int def_flags = SYMBOL_DEF << def;
> > +
> > +       if (!p)
> > +               return;
> the environment should already gives you the guarantee that `p' won't
> be NULL. If that's not the case, the environment is likely to be
> broken.

Yes, that's true. Nevertheless, I've seen broken environments — there's
no harm in being robust. We can't be robust in the face of *all* the
ways the environment could screw up, of course, but this check doesn't
really hurt. Especially given that the getenv() isn't even *in* this
function, so this function is even more justified in checking its
inputs.

> > +
> > +       *p = 0;
> > +       sym = sym_find(str + strlen(CONFIG_));
> > +       *p++ = '=';
> > +
> > +       if (!sym)
> > +               return;
> > +
> > +       sym_calc_value(sym);
> > +       if (!sym_set_string_value(sym, p))
> > +               return;
> > +       conf_message("CONFIG_%s set to %s from environment", sym->name, p);
> please do not hardcode the prefix.

OK.

> > +       if (sym && sym_is_choice_value(sym)) {
> you do not need to test for the validity of a pointer you
> unconditionally dereference right before :)

That's a copy-and-paste from identical code in conf_read_simple(), which
I suppose should be factored out into a separate function. if I really
understood what it was doing I'd be able to suggest a sane name...
sym_validate_choice_state() perhaps?

I'll do that.

> > +               struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym));
> > +               switch (sym->def[def].tri) {
> > +               case no:
> > +                       break;
> > +               case mod:
> > +                       if (cs->def[def].tri == yes) {
> > +                               conf_warning("%s creates inconsistent choice state", sym->name);
> > +                               cs->flags &= ~def_flags;
> > +                       }
> > +                       break;
> > +               case yes:
> > +                       if (cs->def[def].tri != no)
> > +                               conf_warning("override: %s changes choice state", sym->name);
> > +                       cs->def[def].val = sym;
> > +                       break;
> > +               }
> > +               cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
> > +       }
> > +}
> > +
-- 
dwmw2