Re: f2fs crypto: add symlink encryption

[Dan Carpenter <dan.carpenter@oracle.com>]

Hello Jaegeuk Kim,

The patch 24d584edddca: "f2fs crypto: add symlink encryption" from
Apr 29, 2015, leads to the following static checker warning:

	fs/f2fs/namei.c:487 f2fs_symlink()
	warn: calling kfree() when 'sd' is always NULL.

fs/f2fs/namei.c
   484          f2fs_fname_crypto_free_buffer(&disk_link);
   485          return err;
   486  out:
   487          kfree(sd);
                ^^^^^^^^^
Freeing a NULL pointer is harmless but I have a static checker warning
for it because it can indicate confusion or typos.

   488          f2fs_fname_crypto_free_buffer(&disk_link);
   489          handle_failed_inode(inode);
   490          return err;
   491  }

"out" labels are bad.

The name is meaningless.  If it says "goto free_sd;" that's useful and
you know what it does without scrolling down and losing your place.
But with out: labels you can't know.

Sometimes out labels do nothing.

	out:
		return err;

These are supposed to prevent return with lock held bugs etc, but if you
look through the git log it has historically not been effective.  It
also introduces "forgot to set the error code" bugs.

Sometime it does everything like in this case and that's very bug prone.
For example, shouldn't we release f2fs_lock_op(sbi) when f2fs_add_link()
fails earlier?

Sometimes it does one thing which is good, but the name is just lazy.

-out:
+unlock:
	mutext_unlock();
	return err;

Btw shouldn't we earlier if page_symlink() fails?

regards,
dan carpenter


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y