From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752708AbeEULG7 (ORCPT ); Mon, 21 May 2018 07:06:59 -0400 Received: from mail-ot0-f195.google.com ([74.125.82.195]:42825 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752627AbeEULG5 (ORCPT ); Mon, 21 May 2018 07:06:57 -0400 X-Google-Smtp-Source: AB8JxZr6JtMdwTUCxxTrbqKUvQpZpXh1QQEaNX35qT05iyGKNcjXW7VJWAnEww2Ta/7Oap5NPtOIrnrO7C3iqiti9+4= MIME-Version: 1.0 In-Reply-To: References: <1526537830-22606-1-git-send-email-yamada.masahiro@socionext.com> <1526537830-22606-4-git-send-email-yamada.masahiro@socionext.com> From: Ulf Magnusson Date: Mon, 21 May 2018 13:06:56 +0200 Message-ID: Subject: Re: [PATCH v4 03/31] kconfig: reference environment variables directly and remove 'option env=' To: Masahiro Yamada Cc: Linux Kbuild mailing list , Linus Torvalds , Sam Ravnborg , "Luis R . Rodriguez" , Linux Kernel Mailing List , Nicholas Piggin , Kees Cook , Emese Revfy , X86 ML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 21, 2018 at 6:43 AM, Masahiro Yamada wrote: > Hi. > > > > 2018-05-21 0:46 GMT+09:00 Ulf Magnusson : > >> 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