Re: [PATCH 11/27] modpost: use doubly linked list for dump_lists

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
>
> This looks easier to understand (just because this is a pattern in
> the kernel code). No functional change is intended.
>
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
>
>  scripts/mod/modpost.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index e1eb188d6282..4c074d6c1721 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2496,7 +2496,7 @@ static void write_namespace_deps_files(const char *fname)
>  }
>
>  struct dump_list {
> -       struct dump_list *next;
> +       struct list_head list;
>         const char *file;
>  };
>
> @@ -2508,8 +2508,8 @@ int main(int argc, char **argv)
>         char *dump_write = NULL, *files_source = NULL;
>         int opt;
>         int n;
> -       struct dump_list *dump_read_start = NULL;
> -       struct dump_list **dump_read_iter = &dump_read_start;
> +       LIST_HEAD(dump_lists);
> +       struct dump_list *dl, *dl2;
>
>         while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
>                 switch (opt) {
> @@ -2517,10 +2517,9 @@ int main(int argc, char **argv)
>                         external_module = true;
>                         break;
>                 case 'i':
> -                       *dump_read_iter =
> -                               NOFAIL(calloc(1, sizeof(**dump_read_iter)));
> -                       (*dump_read_iter)->file = optarg;
> -                       dump_read_iter = &(*dump_read_iter)->next;
> +                       dl = NOFAIL(malloc(sizeof(*dl)));
> +                       dl->file = optarg;
> +                       list_add_tail(&dl->list, &dump_lists);

Consider NULL'ing out dl since you reuse its storage below.  Either way
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

>                         break;
>                 case 'm':
>                         modversions = true;
> @@ -2554,13 +2553,10 @@ int main(int argc, char **argv)
>                 }
>         }
>
> -       while (dump_read_start) {
> -               struct dump_list *tmp;
> -
> -               read_dump(dump_read_start->file);
> -               tmp = dump_read_start->next;
> -               free(dump_read_start);
> -               dump_read_start = tmp;
> +       list_for_each_entry_safe(dl, dl2, &dump_lists, list) {
> +               read_dump(dl->file);
> +               list_del(&dl->list);
> +               free(dl);
>         }
>
>         while (optind < argc)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers