linux-kernel - Re: [PATCH 3/6] staging: rtl8188eu: remove goto label

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: <20150717114755.GB5422@mwanda>
Date:	Fri, 17 Jul 2015 14:47:55 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	devel@...verdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] staging: rtl8188eu: remove goto label

On Fri, Jul 17, 2015 at 04:55:12PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 17, 2015 at 02:03:48PM +0300, Dan Carpenter wrote:
> > On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> > > By checking for the success of kzalloc we were able to remove the goto
> > > label thus making the code more readable.
> > > 
> > 
> > No...  You've just changed error handling to success handling and added
> > some new indent levels and made a tangled spaghetti exit path even more
> > tangled.  Spoderman wants to know, "Why u do dis?"
> I had to go for google to lookup Spoderman or Spiderman. :)
> At the end of the series, isn't the code better looking and simpler
> than the original code?

Yes, but that's not a very high bar to clear.

What I'm trying to explain is the beauty of writing one statement after
the other without indenting.  The simplest kind of code looks like this
without error handling.


int some_function(void)
{
	do_thing_one();
	do_thing_two();
	do_thing_three();

	return 0;
}

When you add canonical error handling it looks like this:

int some_function(void)
{
	do_thing_one();
	if (failure)
		return -ERROR;

	do_thing_two();
	if (failure)
		goto undo_one;

	do_thing_three();
	if (failure)
		goto undo_two;

	return 0;

undo_two:
	undo_two_thing();
undo_one:
	undo_one_thing();

	return -ERROR;
}

But with success handling it look like this:

int some_function(void)
{
	do_thing_one();
	if (success) {
		do_thing_two();
		if (success) {
			do_thing_three();
			if (success)
				return 0;
		}
	}
	return -ERROR;
}

Fewer lines, but terrible code.  In this patch we only make the last
condition success handling but it's still messier than need be.

	if (fail)
	if (fail)
	if (fail)
	if (success)
	else

The canonical method is more consistent and simplest to read.

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/