Re: [PATCH v2 14/22] staging/rdma/hfi1: Implement Expected Receive TID caching

[Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On Thu, Oct 22, 2015 at 07:18:19PM -0400, ira.weiny wrote:
> This follows the rest of the style of the case statement in this function.  We
> prefer to leave this as is for a number of reasons.
> 
> 1) This is consistent with the coding style elsewhere in this driver.

Don't try to match existing style if it is wrong.  If 99 lines are
consistent and 1 line is correct style then at least that's better than
no lines being correct.  I am worried that you will feel you have to do
this the wrong way forever for a silly reason...

> 2) It is functionally equivalent.

It is a style issue and I only complained about it because in the
next lines the bad style causes a bug.  If anyone finds this kind of
info leak in released code, then we always give them a CVE for it btw.
It's a headache.


> 3) I have a long list of patches which need to be processed and this may cause
>    later merge conflicts.
> 

Yes, that's fine.  I'm not insisting that you redo everything because of
a style issue.

Let me explain a little more why success handling is an anti-pattern.
Failure handling looks like this:

	ret = one();
	if (ret)
		return ret;
	ret = two();
	if (ret)
		goto undo_one;
	ret = three();
	if (ret)
		goto undo_two;

	return 0;

undo_two:
	undo_two();
undo_one:
	undo_one();
	return ret;

In this example the success path is always at indent level one.  The
code is a series of statements with no if conditions or indenting.  This
is how most kernel code looks.

With success handling it looks like:

	ret = one();
	if (!ret) {
		ret = two();
		if (!ret)
			ret = three();
	}
	return ret;

It is fewer lines but it is way more complicated.  It very quickly
starts to bump into the 80 character limit.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html