Re: [PATCH v4 02/17] trailer: process trailers from file and arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 31, 2014, 04:02 UTC
Message-ID: <CAPig+cSaYfsyx2YmxOqLxxYLhpgqhznwOzP5CpY7jw=p4sGgpw@mail.gmail.com>
In-Reply-To: <20140130064921.7504.60878.chriscool@tuxfamily.org>

On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder <chriscool@tuxfamily.org> wrote:

Show 25 quoted lines

> This patch implements the logic that process trailers
> from file and arguments.
>
> At the beginning trailers from file are in their own
> infile_tok doubly linked list, and trailers from
> arguments are in their own arg_tok doubly linked list.
>
> The lists are traversed and when an arg_tok should be
> "applied", it is removed from its list and inserted
> into the infile_tok list.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/trailer.c b/trailer.c
> index aed25e1..e9ccfa5 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -46,3 +46,192 @@ static size_t alnum_len(const char *buf, size_t len)
> +static void apply_arg_if_exist(struct trailer_item *infile_tok,
> +                              struct trailer_item *arg_tok,
> +                              int alnum_len)
> +{
> +       switch (arg_tok->conf->if_exist) {
> +       case EXIST_DO_NOTHING:
> +               free(arg_tok);

This is freeing arg_tok, but isn't it leaking arg_tok->conf, and conf->name, conf->key, conf->command? Ditto for all the other free(arg_tok) invocations elsewhere in the file.

 > +               break;

Show 39 quoted lines

> +       case EXIST_OVERWRITE:
> +               free((char *)infile_tok->value);
> +               infile_tok->value = xstrdup(arg_tok->value);
> +               free(arg_tok);
> +               break;
> +       case EXIST_ADD:
> +               add_arg_to_infile(infile_tok, arg_tok);
> +               break;
> +       case EXIST_ADD_IF_DIFFERENT:
> +               if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
> +                       add_arg_to_infile(infile_tok, arg_tok);
> +               else
> +                       free(arg_tok);
> +               break;
> +       case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
> +               if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
> +                       add_arg_to_infile(infile_tok, arg_tok);
> +               else
> +                       free(arg_tok);
> +               break;
> +       }
> +}
> +
> +static void process_infile_tok(struct trailer_item *infile_tok,
> +                              struct trailer_item **arg_tok_first,
> +                              enum action_where where)
> +{
> +       struct trailer_item *arg_tok;
> +       struct trailer_item *next_arg;
> +
> +       int tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));
> +       for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
> +               next_arg = arg_tok->next;
> +               if (same_token(infile_tok, arg_tok, tok_alnum_len) &&
> +                   arg_tok->conf->where == where) {
> +                       /* Remove arg_tok from list */
> +                       remove_from_list(arg_tok, arg_tok_first);
> +                       /* Apply arg */
> +                       apply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);

Redundant comments (saying the same thing as the code) can make the code slightly more difficult to read.

Show 9 quoted lines

> +                       /*
> +                        * If arg has been added to infile,
> +                        * then we need to process it too now.
> +                        */
> +                       if ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)
> +                               infile_tok = arg_tok;
> +               }
> +       }
> +}