Re: [PATCH 03/20] tools/xenstore: let talloc_free() preserve errno

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 01/11/2022 15:28, Juergen Gross wrote:
> Today talloc_free() is not guaranteed to preserve errno, especially in
> case a custom destructor is being used.
>
> Change that by renaming talloc_free() to _talloc_free() in talloc.c and
> adding a wrapper to talloc.c.
>
> This allows to remove some errno saving outside of talloc.c.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   tools/xenstore/talloc.c         | 25 ++++++++++++++++++-------
>   tools/xenstore/xenstored_core.c |  2 --
>   2 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/tools/xenstore/talloc.c b/tools/xenstore/talloc.c
> index d7edcf3a93..5fbefdf091 100644
> --- a/tools/xenstore/talloc.c
> +++ b/tools/xenstore/talloc.c
> @@ -103,6 +103,8 @@ struct talloc_chunk {
>         unsigned flags;
>   };
>   
> +static int _talloc_free(void *ptr);
> +
>   /* 16 byte alignment seems to keep everyone happy */
>   #define TC_HDR_SIZE ((sizeof(struct talloc_chunk)+15)&~15)
>   #define TC_PTR_FROM_CHUNK(tc) ((void *)(TC_HDR_SIZE + (char*)tc))
> @@ -245,7 +247,7 @@ static int talloc_reference_destructor(void *ptr)
>                 tc1->destructor = NULL;
>         }
>         _TLIST_REMOVE(tc2->refs, handle);
> -       talloc_free(handle);
> +       _talloc_free(handle);
From the commit message, it is not clear to me why we are calling the 
underscore version here. Same for the others below.
>         return 0;
>   }
>   
> @@ -311,7 +313,7 @@ static int talloc_unreference(const void *context, const void *ptr)
>   
>   	talloc_set_destructor(h, NULL);
>         _TLIST_REMOVE(tc->refs, h);
> -       talloc_free(h);
> +       _talloc_free(h);
>         return 0;
>   }
>   
> @@ -349,7 +351,7 @@ int talloc_unlink(const void *context, void *ptr)
>         tc_p = talloc_chunk_from_ptr(ptr);
>   
>   	if (tc_p->refs == NULL) {
> -               return talloc_free(ptr);
> +               return _talloc_free(ptr);
>         }
>   
>   	new_p = talloc_parent_chunk(tc_p->refs);
> @@ -521,7 +523,7 @@ static void talloc_free_children(void *ptr)
>                         struct talloc_chunk *p = 
> talloc_parent_chunk(tc->child->refs);
>                         if (p) new_parent = TC_PTR_FROM_CHUNK(p);
>                 }
> -               if (talloc_free(child) == -1) {
> +               if (_talloc_free(child) == -1) {
>                         if (new_parent == null_context) {
>                                 struct talloc_chunk *p = 
> talloc_parent_chunk(ptr);
>                                 if (p) new_parent = TC_PTR_FROM_CHUNK(p);
> @@ -539,7 +541,7 @@ static void talloc_free_children(void *ptr)
>      will not be freed if the ref_count is > 1 or the destructor (if
>      any) returns non-zero
Can you expand this comment to explain the different between 
_talloc_free() and talloc_free()?
I agree the code is probably clear enough, but better to be obvious.

>   */
> -int talloc_free(void *ptr)
> +static int _talloc_free(void *ptr)
>   {
>         struct talloc_chunk *tc;
>   
> @@ -597,7 +599,16 @@ int talloc_free(void *ptr)
>         return 0;
>   }
>   
> +int talloc_free(void *ptr)
> +{
> +       int ret;
> +       int saved_errno = errno;
>   
> +	ret = _talloc_free(ptr);
> +       errno = saved_errno;
> +
> +       return ret;
> +}
>   
>   /*
>     A talloc version of realloc. The context argument is only used if
> @@ -610,7 +621,7 @@ void *_talloc_realloc(const void *context, void *ptr, 
> size_t size, const char *n
>   
>   	/* size zero is equivalent to free() */
>         if (size == 0) {
> -               talloc_free(ptr);
> +               _talloc_free(ptr);
>                 return NULL;
>         }
>   
> @@ -1243,7 +1254,7 @@ void *talloc_realloc_fn(const void *context, void *ptr, size_t size)
>   
>   static void talloc_autofree(void)
>   {
> -       talloc_free(cleanup_context);
> +       _talloc_free(cleanup_context);
>         cleanup_context = NULL;
>   }
>   
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 476d5c6d51..5a174b9881 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;
>   }
>   
Cheers,

--
Julien Grall