Re: [PATCH RFC] libtraceevent: Improve exception handling in parse_arg_add()

Re: [PATCH RFC] libtraceevent: Improve exception handling in parse_arg_add()

[Steven Rostedt]

On Sun, 12 Mar 2023 10:02:24 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> Hello,
> 
> I hope that the advice “If there is no cleanup needed then just return directly.”

                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


> can be taken better into account.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.3-rc1#n532
> 
> Another improvable implementation detail was detected by the means of
> the semantic patch language (Coccinelle software).
> How will the chances evolve to integrate the following change suggestion?
> 
> 
> diff --git a/src/event-parse.c b/src/event-parse.c
> index e655087..1ca5fdd 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c
> @@ -6388,22 +6388,20 @@ static int parse_arg_add(struct tep_print_parse **parse, char *format,
>  
>      parg = calloc(1, sizeof(*parg));
>      if (!parg)
> -        goto error;
> +        return -1;
> +
>      parg->format = strdup(format);
> -    if (!parg->format)
> -        goto error;
> +    if (!parg->format) {
> +        free(parg);
           ^^^^^^^^^^

That's clean up.

-- Steve

> +        return -1;
> +    }
> +
>      parg->type = type;
>      parg->arg = arg;
>      parg->len_as_arg = len_as_arg;
>      parg->ls = ls;
>      *parse = parg;
>      return 0;
> -error:
> -    if (parg) {
> -        free(parg->format);
> -        free(parg);
> -    }
> -    return -1;
>  }
>  
>  static int parse_arg_format(struct tep_print_parse **parse,
> 
> 
> Regards,
> Markus

Re: [PATCH RFC] libtraceevent: Improve exception handling in parse_arg_add()

[Steven Rostedt]

On Mon, 13 Mar 2023 21:35:38 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> >> diff --git a/src/event-parse.c b/src/event-parse.c
> >> index e655087..1ca5fdd 100644
> >> --- a/src/event-parse.c
> >> +++ b/src/event-parse.c
> >> @@ -6388,22 +6388,20 @@ static int parse_arg_add(struct tep_print_parse **parse, char *format,
> >>  
> >>      parg = calloc(1, sizeof(*parg));
> >>      if (!parg)
> >> -        goto error;
> >> +        return -1;
> >> +  
> 
> The first failed memory allocation does not need clean-up.
> 
> 
> >>      parg->format = strdup(format);
> >> -    if (!parg->format)
> >> -        goto error;
> >> +    if (!parg->format) {
> >> +        free(parg);  
> >            ^^^^^^^^^^
> >
> > That's clean up.  
> 
> Yes. ‒ I propose to move a single memory release into an if branch
> as a known design approach for better exception handling.

This has nothing to do with clean ups, but personal preferences. Neither is
more correct than the other. Both work. I prefer the code as is. End of
discussion.

-- Steve