[SSSD] [PATCH] Remove useless assignment to function parameter
Simo Sorce
simo at redhat.com
Wed Feb 26 18:46:16 UTC 2014
On Wed, 2014-02-26 at 19:36 +0100, Lukas Slebodnik wrote:
> On (26/02/14 13:00), Simo Sorce wrote:
> >On Wed, 2014-02-26 at 18:33 +0100, Lukas Slebodnik wrote:
> >> ehlo,
> >>
> >> Reported by: cppcheck
> >>
> >> void free_fun(struct info *info)
> >> free(info->name);
> >> free(info);
> >> info = NULL;
> >> ^^^^^^^^^^^
> >> Assignment to function parameter has no effect outside the function.
> >>
> >> Patch is attached.
> >
> >Although the assignment is useless I do not like the solution.
> >
> >I think the solution is not to remove the assignment but to change the
> >function prototype to take a pointer to the pointer we want to free so
> >that at the end of the function we can assign NULL to the original
> >variable we have been called to free.
> >Otherwise the caller has a dangling pointer.
> >
> >So the solution for me is:
> >void free_fun(struct info **_info) {
> > struct info *info = *_info;
> > free(info->name);
> > free(info);
> > *info = NULL;
> >}
> >
> >Simo.
> >
> I do not agree with you.
>
> Free functions have parameter (void *) which will be freed.
> e.g.:
> void free(void *ptr);
> int talloc_free (void *ptr)
> /**
> * Free a string allocated by a krb5 function.
> *
> * @param [in] context Library context
> * @param [in] val String to be freed
> *
> * @version First introduced in 1.10
> */
> void KRB5_CALLCONV
> krb5_free_string(krb5_context context, char *val);
>
> If you do not want to have dangling pointer, you need to assign NULL yourself
> or use macro.
gssapi OTOH uses th convention that you have to pass a pointer and then
NULLs the one you passed int, and I find it much less prone to mistakes.
> #define talloc_zfree(ptr) \
> do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0)
Note that it is common agreement even upstream (I think) that this is an
unfortunate side-effect of not having thought about doing this right
beofre the API was finalized.
Having to define macros is not a good idea in general, it adds
unnecessary churn.
> I do not want to introduce new "semantic" to free functions.
They are not new in any way, a lot of APIs use the semantic I recommend
and it is a better one, in general.
> In many cases, you cannot change API of free function, because they are used
> as a callback.
If there is a technical issue that prevents changing a specific function
then that's fine, but I prefer the way I shown as a general solution, I
started using it quite a while ago in all projects I am active in.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the sssd-devel
mailing list