linux-kernel - Re: [PATCH 09/50] staging: omap-thermal: make a omap_bandgap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130316135618.GI9189@mwanda>
Date:	Sat, 16 Mar 2013 16:56:18 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Eduardo Valentin <eduardo.valentin@...com>
Cc:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH 09/50] staging: omap-thermal: make a omap_bandgap_power
 with only one exit point

On Sat, Mar 16, 2013 at 08:39:11AM -0400, Eduardo Valentin wrote:
> Hey Dan,
> 
> On 15-03-2013 17:22, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote:
> >>Change the way the omap_bandgap_power is written so that it has only
> >>one exit entry (Documentation/CodingStyle).
> >>
> >
> >It's only if there is an unlock or something that you should do
> >this.  Otherwise the pointless bunny hop is misleading and annoying.
> 
> Well, if that is the case the Chapter 7 needs to be rewritten, don't
> you think? The way it is stated, it is clear that it is a design
> decision to use it for keeping only one exit point (quoting):
> 
> "Albeit deprecated by some people, the equivalent of the goto statement is
> used frequently by compilers in form of the unconditional jump instruction.
> 
> The goto statement comes in handy when a function exits from multiple
> locations and some common work such as cleanup has to be done.
> 
> The rationale is:
> 
> - unconditional statements are easier to understand and follow
> - nesting is reduced
> - errors by not updating individual exit points when making
>     modifications are prevented
> - saves the compiler work to optimize redundant code away ;)"
> 
> I believe this patch falls into at least three of the above rationale.

There isn't any cleanup work done.  That's what I was explaining
about.  When I see a goto I expect cleanup work to be done, but in
this case it was misleading.

Where you would do the one exit point is when there is cleanup:

	ret = function_one();
	if (ret)
		goto unlock;

	ret = function_two();
	if (ret)
		goto unlock;


That's better than:
	ret = function_one();
	if (ret) {
		mutex_unlock();
		return ret;
	}

	ret = function_two();
	if (ret) {
		mutex_unlock();
		return ret;
	}

On the one hand, I think the documentation should be updated because
obviously people get confused by it.  But on the other hand, to me
the documentation is already pretty clear.  There is no style
guideline that says every function should only have a single return.
That would be silly.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/