Kaleb KEITHLEY (Code Review) wrote: > Kaleb KEITHLEY has posted comments on this change. > > Change subject: nfs: memory leak fixes > ...................................................................... > > > Patch Set 1: (3 inline comments) > > .................................................... > File xlators/nfs/server/src/nfs-common.c > Line 325: GF_FREE (path); > I notice that in other calls to > inode_path()/nfs_loc_fill()/GF_FREE(path) that path (or resolvedpath) > is checked for non-null before the GF_FREE(), even though it looks > like inode_path() must have allocated memory if it didn't return an > error. ... > To view, visit http://review.gluster.com/3641 Hi guys, Any if-before-GF_FREE is almost certainly wasted effort, since GF_FREE already tests for NULL pointers. The added conditional generally makes the code a little harder to read/maintain. (i.e., above) What do you think about removing all of them? If there's interest, I'd be happy to do it. Running my little useless-if-before-free (find and/or remove) http://git.sv.gnu.org/cgit/gnulib.git/tree/build-aux/useless-if-before-free script, I see that there are nearly 200 of those when considering solely if-before-"free". Considering GF_FREE, I see an additional 700+ useless tests. Some argue that it's best to test before free to avoid the overhead of a function call, but so far I've never seen such a claim backed up by real profiling data. Jim PS. Can you tell I've done this a few times? Here are some of the projects that have endured this janitorial work: git gcc emacs glibc gnulib coreutils freeIPA libvirt util-linux-ng idutils openais corosync PPS, Once clean, several of the above projects started using a make rule (we call them syntax-check rules) to ensure that no additional if-before-<free-like-function> is introduced. If there's interest, I can add that, too.
