Re: [PATCH v3 01/17] tools/xenstore: let talloc_free() preserve errno

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 17/01/2023 09:11, Juergen Gross wrote:
> Today talloc_free() is not guaranteed to preserve errno, especially in
> case a custom destructor is being used.
>
> So preserve errno in talloc_free().
>
> This allows to remove some errno saving outside of talloc.c.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

> ---
> V2:
> - drop wrapper (Julien Grall)
> ---
>   tools/xenstore/talloc.c         | 21 +++++++++++++--------
>   tools/xenstore/xenstored_core.c |  2 --
>   2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/tools/xenstore/talloc.c b/tools/xenstore/talloc.c
> index d7edcf3a93..23c3a23b19 100644
> --- a/tools/xenstore/talloc.c
> +++ b/tools/xenstore/talloc.c
> @@ -541,38 +541,39 @@ static void talloc_free_children(void *ptr)
>   */
>   int talloc_free(void *ptr)
>   {
> +       int saved_errno = errno;
>         struct talloc_chunk *tc;
>   
>   	if (ptr == NULL) {
> -               return -1;
> +               goto err;
>         }
>   
>   	tc = talloc_chunk_from_ptr(ptr);
>   
>   	if (tc->null_refs) {
>                 tc->null_refs--;
> -               return -1;
> +               goto err;
>         }
>   
>   	if (tc->refs) {
>                 talloc_reference_destructor(tc->refs);
> -               return -1;
> +               goto err;
>         }
>   
>   	if (tc->flags & TALLOC_FLAG_LOOP) {
>                 /* we have a free loop - stop looping */
> -               return 0;
> +               goto success;
>         }
>   
>   	if (tc->destructor) {
>                 talloc_destructor_t d = tc->destructor;
>                 if (d == (talloc_destructor_t)-1) {
> -                       return -1;
> +                       goto err;
>                 }
>                 tc->destructor = (talloc_destructor_t)-1;
>                 if (d(ptr) == -1) {
>                         tc->destructor = d;
> -                       return -1;
> +                       goto err;
>                 }
>                 tc->destructor = NULL;
>         }
> @@ -594,10 +595,14 @@ int talloc_free(void *ptr)
>         tc->flags |= TALLOC_FLAG_FREE;
>   
>   	free(tc);
> + success:
> +       errno = saved_errno;
>         return 0;
> -}
> -
>   
> + err:
> +       errno = saved_errno;
> +       return -1;
> +}
>   
>   /*
>     A talloc version of realloc. The context argument is only used if
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 78a3edaa4e..1650821922 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -771,9 +771,7 @@ struct node *read_node(struct connection *conn, const void 
> *ctx,
>         return node;
>   
>    error:
> -       err = errno;
>         talloc_free(node);
> -       errno = err;
>         return NULL;
>   }
>   
--
Julien Grall