Re: [PATCH v2] scripts/kconfig: cleanup symbol handling code

[Masahiro Yamada]

2018-03-10 19:56 GMT+09:00 Joey Pabalinas <joeypabalinas@xxxxxxxxx>:
> 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 <ulfalizer@xxxxxxxxx>
> Signed-off-by: Joey Pabalinas <joeypabalinas@xxxxxxxxx>
>
>  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 <ctype.h>
>  #include <stdlib.h>
> -#include <string.h>
>  #include <regex.h>
> +#include <linux/string.h>
>  #include <sys/utsname.h>


<string.h> 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