thom311 / libnl

Netlink Library Suite
GNU Lesser General Public License v2.1
419 stars 311 forks source link

[th/xfrm-addr-cleanup] some cleanup to XFRM code related to nl_addr #367

Closed thom311 closed 9 months ago

thom311 commented 9 months ago

@spellingmistake seeing that you did some XFRM patches in the past, what do you think of this?

spellingmistake commented 9 months ago

My answer is, does te/xfrm-addr-cleanup go too far?

thom311 commented 9 months ago

too far in which respect? :)

spellingmistake commented 9 months ago

Anything that removes those goto-statements is welcome. An as portability is not an issue, the cleanup-attribute is the way to go. Some nifty way to get around forgetting the = NULL part would be nice. So far, I've used something like this:

#define SOMECLEANUP __attribute__((cleanup(some_clean_func))) = NULL

which I use as follows

some_type_t *stp SOMECLEANUP;

thom311 commented 9 months ago

I think forgetting to initialize the cleanup variable is unlikely to be an issue.

The compiler will warn about uninitialized variables in most cases. Especially when building with -fexceptions (which for example is in the default CFLAGS for Fedora builds), basically all uninitialized cleanup-variables will be warned, even if by review you would assume that the variable gets initialized before the first return.

thom311 commented 9 months ago

some_type_t *stp SOMECLEANUP;

Not bad. It seems possible too. No strong opinion, except that the way the cleanup macros in libnl are now similar to how they are in NetworkManager and glib (in glib there is also g_auto(Type) pattern). That's why I am more familiar with the current style.

And the danger of uninitialized variables seems low as the compiler provides useful warnings (and we build with -Werror).