Date:   Mon, 21 May 2018 13:06:56 +0200
From:   Ulf Magnusson <ulfalizer@...il.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Sam Ravnborg <sam@...nborg.org>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Emese Revfy <re.emese@...il.com>, X86 ML <x86@...nel.org>
Subject: Re: [PATCH v4 03/31] kconfig: reference environment variables
 directly and remove 'option env='

On Mon, May 21, 2018 at 6:43 AM, Masahiro Yamada
<yamada.masahiro@...ionext.com> wrote:
> Hi.
>
>
>
> 2018-05-21 0:46 GMT+09:00 Ulf Magnusson <ulfalizer@...il.com>:
>
>> s/environments/environment variables/
>
> Will fix.
>
>
>>
>>> +        * They will be written out to include/config/auto.conf.cmd
>>> +        */
>>> +       env_add(name, value);
>>> +
>>> +       return xstrdup(value);
>>> +}
>>> +
>>> +void env_write_dep(FILE *f, const char *autoconfig_name)
>>> +{
>>> +       struct env *e, *tmp;
>>> +
>>> +       list_for_each_entry_safe(e, tmp, &env_list, node) {
>>> +               fprintf(f, "ifneq \"$(%s)\" \"%s\"\n", e->name, e->value);
>>> +               fprintf(f, "%s: FORCE\n", autoconfig_name);
>>> +               fprintf(f, "endif\n");
>>> +               env_del(e);
>>> +       }
>>> +}
>>> +
>>> +static char *eval_clause(const char *in)
>>> +{
>>> +       char *res, *name;
>>> +
>>> +       /*
>>> +        * Returns an empty string because '$()' should be evaluated
>>> +        * to a null string.
>>> +        */
>>
>> Do you know of cases where this is more useful than erroring out?
>>
>> Not saying it doesn't make sense. Just curious.
>
>
> I just followed how Make works.
>
> Anyway, eval_clause() will return null string for null input.
> I will remove that hunk.
>
>
>
>
>>> +       if (!*in)
>>> +               return xstrdup("");
>>> +
>>> +       name = expand_string(in);
>>> +
>>> +       res = env_expand(name);
>>> +       free(name);
>>> +
>>> +       return res;
>>> +}
>>> +
>>> +/*
>>> + * Expand a string that follows '$'
>>> + *
>>> + * For example, if the input string is
>>> + *     ($(FOO)$($(BAR)))$(BAZ)
>>> + * this helper evaluates
>>> + *     $($(FOO)$($(BAR)))
>>> + * and returns the resulted string, then updates 'str' to point to the next
>>
>> s/resulted/resulting/
>>
>> Maybe something like this would make the behavior a bit clearer:
>>
>>   ...and returns a new string containing the expansion, also advancing
>>   'str' to point to the next character after... (note that this function does
>>   a recursive expansion) ...
>
>
> Is this OK?
>
> /*
>  * Expand a string that follows '$'
>  *
>  * For example, if the input string is
>  *     ($(FOO)$($(BAR)))$(BAZ)
>  * this helper evaluates
>  *     $($(FOO)$($(BAR)))
>  * and returns a new string containing the expansion (note that the string is
>  * recursively expanded), also advancing 'str' to point to the next character
>  * after the corresponding closing parenthesis, in this case, *str will be
>  *     $(BAR)
>  */

Looks fine to me.

>
>
>
>>> + * character after the corresponding closing parenthesis, in this case, *str
>>> + * will be
>>> + *     $(BAR)
>>> + */
>>> +char *expand_dollar(const char **str)
>>> +{
>>> +       const char *p = *str;
>>> +       const char *q;
>>> +       char *tmp, *out;
>>> +       int nest = 0;
>>> +
>>> +       /* '$$' represents an escaped '$' */
>>> +       if (*p == '$') {
>>> +               *str = p + 1;
>>> +               return xstrdup("$");
>>> +       }
>>> +
>>> +       /*
>>> +        * Kconfig does not support single-letter variable as in $A
>>> +        * or curly braces as in ${CC}.
>>> +        */
>>> +       if (*p != '(')
>>> +               pperror("syntax error: '$' not followed by '('", p);
>>
>> Could say "not followed by '(' by or '$'".
>
> Will do.
>
>
>>> +
>>> +       p++;
>>> +       q = p;
>>> +       while (*q) {
>>> +               if (*q == '(') {
>>> +                       nest++;
>>> +               } else if (*q == ')') {
>>> +                       if (nest-- == 0)
>>> +                               break;
>>> +               }
>>> +               q++;
>>> +       }
>>> +
>>> +       if (!*q)
>>> +               pperror("unterminated reference to '%s': missing ')'", p);
>>> +
>>> +       tmp = xmalloc(q - p + 1);
>>> +       memcpy(tmp, p, q - p);
>>> +       tmp[q - p] = 0;
>>> +       *str = q + 1;
>>> +       out = eval_clause(tmp);
>>> +       free(tmp);
>>> +
>>> +       return out;
>>
>> This might be a bit clearer, since the 'str' update and the expansion
>> are independent:
>
> Indeed, will do.
>
>>
>>   /* Advance 'str' to after the expanded initial portion of the string */
>>   *str = q + 1;
>>
>>   /* Save the "FOO" part of "(FOO)" into 'tmp' and expand it recursively */
>>   tmp = xmalloc(q - p + 1);
>>   memcpy(tmp, p, q - p);
>>   tmp[q - p] = '\0';
>>   out = eval_clause(tmp);
>>   free(tmp);
>>
>>   return out;
>>
>> ...or switched around, but thought putting the 'str' bit first might
>> emphasize that it isn't modified.
>
>
> I prefer advancing the pointer at last.

Yeah, it's a bit less weird in a way...

>
>
>
>
>>> +}
>>> +
>>> +/*
>>> + * Expand variables in the given string.  Undefined variables
>>> + * expand to an empty string.
>>
>> I wonder what the tradeoffs are vs. erroring out here (or leaving
>> undefined variables as-is).
>
>
> I want to stick to the Make-like behavior here and exploit it in some cases.
> We can set some variables in some arch/*/Kconfig,
> but unset in the others.
>
>
>
>
> In some arch/*/Kconfig:
>
> min-gcc-version := 40900
>
>
>
>
> In init/Kconfig:
>
> # GCC 4.5 is required to build Linux Kernel.
> # Some architectures may override it (from arch/*/Kconfig) for their
> requirement.
> min-gcc-version := $(if,$(min-gcc-version),$(min-gcc-version),40500)
>
> ... check the gcc version, then show error
> if it is older than $(min-gcc-version).

OK, makes sense. Fine by me.

>>> + * The returned string must be freed when done.
>>> + */
>>> +char *expand_string(const char *in)
>>> +{
>>> +       const char *prev_in, *p;
>>> +       char *new, *out;
>>> +       size_t outlen;
>>> +
>>> +       out = xmalloc(1);
>>> +       *out = 0;
>>> +
>>> +       while ((p = strchr(in, '$'))) {
>>> +               prev_in = in;
>>> +               in = p + 1;
>>> +               new = expand_dollar(&in);
>>> +               outlen = strlen(out) + (p - prev_in) + strlen(new) + 1;
>>> +               out = xrealloc(out, outlen);
>>> +               strncat(out, prev_in, p - prev_in);
>>> +               strcat(out, new);
>>> +               free(new);
>>
>> Some code duplication with expand_one_token() here. Do you think
>> something could be factored out/reorganized?
>
>
> Yes.  For example, the following would work.
> If you prefer that, I can update it in v5.
>
>
>
>
> static char *__expand_string(const char **str, bool (*is_end)(const char *))
> {
>         const char *in, *prev_in, *p;
>         char *new, *out;
>         size_t outlen;
>
>         out = xmalloc(1);
>         *out = 0;
>
>         p = in = *str;
>
>         while (1) {
>                 if (*p == '$') {
>                         prev_in = in;
>                         in = p + 1;
>                         new = expand_dollar(&in);
>                         outlen = strlen(out) + (p - prev_in) + strlen(new) + 1;
>                         out = xrealloc(out, outlen);
>                         strncat(out, prev_in, p - prev_in);
>                         strcat(out, new);
>                         free(new);
>                         p = in;
>                         continue;
>                 }
>
>                 if (is_end(p))
>                         break;
>
>                 p++;
>         }
>
>         outlen = strlen(out) + (p - in) + 1;
>         out = xrealloc(out, outlen);
>         strncat(out, in, p - in);
>
>         *str = p;
>
>         return out;
> }
>
> static bool is_end_of_str(const char *s)
> {
>         return !*s;
> }
>
> char *expand_string(const char *in)
> {
>         return __expand_string(&in, is_end_of_str);
> }
>
> static bool is_end_of_token(const char *s)
> {
>         return !(isalnum(*s) || *s == '_' || *s == '-' || *s == '.' ||
> *s == '/');
> }
>
> char *expand_one_token(const char **str)
> {
>         return __expand_string(str, is_end_of_token);
> }

Yep, something like that would be nicer I think.

This variant might work too (untested):

  dollar_i = p;
  p++;
  expansion = expand_dollar(&p);

  out = xrealloc(out, strlen(out) + (dollar_i - in)
                      + strlen(expansion) + 1);
  strncat(out, in, dollar_i - in);
  strcat(out, expansion);
  free(expansion);

  in = p;

  continue;

The p++ would disappear if expand_dollar() took a pointer to the '$'.

Having some string buffer helpers might be nice, but definitely out of scope.

>
>
>
>
>
>> I wonder if it might be cleaner to have expand_dollar() take a pointer
>> to the '$'. Might even out in terms of the +1's you'd have to add, as
>> all callers manually bump the pointer before the call now. I haven't
>> tried implementing it though, so hard to say.
>
>
> If I do this, I'd want to add a sanity check to expand_dollar().
> I'd rather like to avoid unneeded code.
>
>
> static char *expand_dollar(...)
> {
>         assert(*p == '$');
>         p++;
>
>         /* '$$' represents an escaped '$' */
>         if (*p == '$') {
>                *str = p + 1;
>                return xstrdup("$");
>         }
>
>          ...
>
>
> }

Felt a bit more direct to have it point to the start of the thing
being expanded, and the assert() doesn't seem horrible there to me,
but your call.

>
>
>>
>> Some short inline comments similar to above would help too I think.
>>
>
>
>
> --
> Best Regards
> Masahiro Yamada

Cheers,
Ulf

Powered by blists - more mailing lists