[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