[SSSD] [PATCH] Remove useless assignment to function parameter
Lukas Slebodnik
lslebodn at redhat.com
Wed Feb 26 19:18:49 UTC 2014
On (26/02/14 13:46), Simo Sorce wrote:
>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.
>
It is a workaroud if you cannot change API and don't want to make mistakes.
>> 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.
>
I like this idea.
>> 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.
>
1. sssd_krb5_locator_close is callback in krb5plugin_service_locate_ftable
krb5 locator plugin
2. free_exp_data is callback used by function pam_set_data
Only function hbac_free_info can be changed; but it will be change of API
in ipa/ipa_hbac.h. In this case, I would prefer to file a ticket
and to do similar change also in libsss_idmap library
{sss_idmap_free, sss_idmap_free_sid, sss_idmap_free_dom_sid,
sss_idmap_free_smb_sid, sss_idmap_free_bin_sid}
LS
More information about the sssd-devel
mailing list