From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753672AbeCYQxW (ORCPT ); Sun, 25 Mar 2018 12:53:22 -0400 Received: from conssluserg-02.nifty.com ([210.131.2.81]:29266 "EHLO conssluserg-02.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753632AbeCYQxS (ORCPT ); Sun, 25 Mar 2018 12:53:18 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-02.nifty.com w2PGr7sX011448 X-Nifty-SrcIP: [209.85.213.50] X-Google-Smtp-Source: AG47ELumjYgqlnzbIahK0lhPpsaUSk/7b03bmlcCetiqkH2kLEO3ebdBFZhjDHvgYWbpyEC9bWTR47r1rUzZ2OPdSa4= MIME-Version: 1.0 In-Reply-To: <20180310105659.7e34lk4kvcbl64lg@gmail.com> References: <20180310105659.7e34lk4kvcbl64lg@gmail.com> From: Masahiro Yamada Date: Mon, 26 Mar 2018 01:52:26 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] scripts/kconfig: cleanup symbol handling code To: Joey Pabalinas Cc: Linux Kbuild mailing list , Ulf Magnusson , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2018-03-10 19:56 GMT+09:00 Joey Pabalinas : > Many of the variable names in scripts/kconfig/symbol.c are > far too terse to the point of not at all identifying _what_ > they are actually used for (`p` and `l` as a couple examples), > and overall there is a large amount of code that could use > some cleaning up. > > Give more explicit names to these variables, fix a couple cases > where different variables were sharing the same name and shadowing > each other, and overall cleanup a bit of the messiness in > sym_expand_string_value() and sym_escape_string_value() > while maintaining equivalent program behavior. > > Suggested-by: Ulf Magnusson > Signed-off-by: Joey Pabalinas > > 1 file changed, 69 insertions(+), 61 deletions(-) First of all, I can not compile Kconfig with this patch. masahiro@grover:~/workspace/linux$ make defconfig HOSTLD scripts/kconfig/conf scripts/kconfig/zconf.tab.o: In function `sym_expand_string_value': zconf.tab.c:(.text+0x81a2): undefined reference to `strscpy' zconf.tab.c:(.text+0x8287): undefined reference to `strscpy' zconf.tab.c:(.text+0x82b5): undefined reference to `strscpy' scripts/kconfig/zconf.tab.o: In function `sym_escape_string_value': zconf.tab.c:(.text+0x869b): undefined reference to `strscpy' scripts/kconfig/zconf.tab.o: In function `menu_add_option': zconf.tab.c:(.text+0x99b7): undefined reference to `likely' collect2: error: ld returned 1 exit status scripts/Makefile.host:99: recipe for target 'scripts/kconfig/conf' failed make[1]: *** [scripts/kconfig/conf] Error 1 Makefile:512: recipe for target 'defconfig' failed make: *** [defconfig] Error 2 > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > index 2220bc4b051bd914e3..9ee32ddb44e193719c 100644 > --- a/scripts/kconfig/symbol.c > +++ b/scripts/kconfig/symbol.c > @@ -5,8 +5,8 @@ > > #include > #include > -#include > #include > +#include > #include is correct. I want to see Kconfig improvements in a bigger picture. The changes below are noise. > #include "lkc.h" > @@ -337,7 +337,7 @@ void sym_calc_value(struct symbol *sym) > { > struct symbol_value newval, oldval; > struct property *prop; > - struct expr *e; > + struct expr *expr; > > if (!sym) > return; > @@ -469,7 +469,7 @@ void sym_calc_value(struct symbol *sym) > struct symbol *choice_sym; > > prop = sym_get_choice_prop(sym); > - expr_list_for_each_sym(prop->expr, e, choice_sym) { > + expr_list_for_each_sym(prop->expr, expr, choice_sym) { > if ((sym->flags & SYMBOL_WRITE) && > choice_sym->visible != no) > choice_sym->flags |= SYMBOL_WRITE; > @@ -899,94 +899,100 @@ struct symbol *sym_find(const char *name) > * name to be expanded shall be prefixed by a '$'. Unknown symbol expands to > * the empty string. > */ > -char *sym_expand_string_value(const char *in) > +char *sym_expand_string_value(const char *src) > { > - const char *src; > - char *res; > - size_t reslen; > + const char *in; > + char *res, *out; > + size_t res_len, src_len; > > /* > - * Note: 'in' might come from a token that's about to be > + * Note: 'src' might come from a token that'src about to be > * freed, so make sure to always allocate a new string > */ > - reslen = strlen(in) + 1; > - res = xmalloc(reslen); > - res[0] = '\0'; > + res_len = strlen(src) + 1; > + res = xmalloc(res_len); > + out = res; > > - while ((src = strchr(in, '$'))) { > + while ((in = strchr(src, '$'))) { > char *p, name[SYMBOL_MAXLENGTH]; > - const char *symval = ""; > + const char *sym_val = ""; > struct symbol *sym; > - size_t newlen; > + size_t new_len, sym_len; > > - strncat(res, in, src - in); > - src++; > + strscpy(out, src, in - src); > + out += in - src; > + in++; > > p = name; > - while (isalnum(*src) || *src == '_') > - *p++ = *src++; > + while (isalnum(*in) || *in == '_') > + *p++ = *in++; > *p = '\0'; > > sym = sym_find(name); > if (sym != NULL) { > sym_calc_value(sym); > - symval = sym_get_string_value(sym); > + sym_val = sym_get_string_value(sym); > } > > - newlen = strlen(res) + strlen(symval) + strlen(src) + 1; > - if (newlen > reslen) { > - reslen = newlen; > - res = xrealloc(res, reslen); > + sym_len = strlen(sym_val); > + new_len = sym_len + strlen(res) + strlen(in) + 1; > + if (new_len > res_len) { > + res_len = new_len; > + res = xrealloc(res, res_len); > } > > - strcat(res, symval); > - in = src; > + strscpy(out, sym_val, sym_len); > + out += sym_len; > + src = in; > } > - strcat(res, in); > + src_len = strlen(src); > + strscpy(out, src, src_len); > + out += src_len; > + *out = '\0'; > > return res; > } > > -const char *sym_escape_string_value(const char *in) > +const char *sym_escape_string_value(const char *src) > { > - const char *p; > - size_t reslen; > - char *res; > - size_t l; > + const char *in; > + size_t res_len, in_len; > + char *res, *out; > > - reslen = strlen(in) + strlen("\"\"") + 1; > + res_len = strlen(src) + strlen("\"\"") + 1; > > - p = in; > + in = src; > for (;;) { > - l = strcspn(p, "\"\\"); > - p += l; > + in_len = strcspn(in, "\"\\"); > + in += in_len; > > - if (p[0] == '\0') > + if (*in == '\0') > break; > > - reslen++; > - p++; > + res_len++; > + in++; > } > > - res = xmalloc(reslen); > - res[0] = '\0'; > + res = xmalloc(res_len); > + out = res; > + *out++ = '\"'; > > - strcat(res, "\""); > - > - p = in; > + in = src; > for (;;) { > - l = strcspn(p, "\"\\"); > - strncat(res, p, l); > - p += l; > + in_len = strcspn(in, "\"\\"); > + strscpy(out, in, in_len); > + in += in_len; > + out += in_len; > > - if (p[0] == '\0') > + if (*in == '\0') > break; > > - strcat(res, "\\"); > - strncat(res, p++, 1); > + *out++ = '\\'; > + *out++ = *in++; > } > + *out++ = '\"'; > + *out = '\0'; > > - strcat(res, "\""); > return res; > } > > @@ -1014,8 +1020,8 @@ static int sym_rel_comp(const void *sym1, const void *sym2) > * exactly; if this is the case, we can't decide which comes first, > * and we fallback to sorting alphabetically. > */ > - exact1 = (s1->eo - s1->so) == strlen(s1->sym->name); > - exact2 = (s2->eo - s2->so) == strlen(s2->sym->name); > + exact1 = (s1->eo - s1->so) == (off_t)strlen(s1->sym->name); > + exact2 = (s2->eo - s2->so) == (off_t)strlen(s2->sym->name); > if (exact1 && !exact2) > return -1; > if (!exact1 && exact2) > @@ -1390,31 +1396,33 @@ const char *prop_get_type_name(enum prop_type type) > return "unknown"; > } > > -static void prop_add_env(const char *env) > +static void prop_add_env(const char *env_key) > { > struct symbol *sym, *sym2; > struct property *prop; > - char *p; > + char *env_val; > > sym = current_entry->sym; > sym->flags |= SYMBOL_AUTO; > for_all_properties(sym, prop, P_ENV) { > sym2 = prop_get_symbol(prop); > - if (strcmp(sym2->name, env)) > + if (strcmp(sym2->name, env_key)) > menu_warn(current_entry, "redefining environment symbol from %s", > sym2->name); > return; > } > > prop = prop_alloc(P_ENV, sym); > - prop->expr = expr_alloc_symbol(sym_lookup(env, SYMBOL_CONST)); > + prop->expr = expr_alloc_symbol(sym_lookup(env_key, SYMBOL_CONST)); > > sym_env_list = expr_alloc_one(E_LIST, sym_env_list); > sym_env_list->right.sym = sym; > + env_val = getenv(env_key); > > - p = getenv(env); > - if (p) > - sym_add_default(sym, p); > - else > - menu_warn(current_entry, "environment variable %s undefined", env); > + if (likely(env_val)) { > + sym_add_default(sym, env_val); > + return; > + } > + > + menu_warn(current_entry, "environment variable %s undefined", env_key); > } > -- > 2.16.2 -- Best Regards Masahiro Yamada