| Message ID | 20221101152842.4257-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
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@suse.com> > --- > 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,
On 06.11.22 22:08, Julien Grall wrote: > 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@suse.com> >> --- >> 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. I was targeting only talloc_free() calls from xenstored to preserve errno. I can see your point that we could just do the same for talloc internal calls, preserving errno in other cases, too. OTOH the only relevant case would be the call from talloc_unlink() via talloc_unreference(), which is at least currently no problem regarding errno. Do you have any preferences? I'm leaning towards dropping the wrapper and do the errno preserving just inside talloc_free(). Juergen
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); 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 */ -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; }
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@suse.com> --- tools/xenstore/talloc.c | 25 ++++++++++++++++++------- tools/xenstore/xenstored_core.c | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)