Re: [PATCH] mmc: vub300: Use common code in __download_offload_pseudocode()

[Dan Carpenter]

On Mon, Oct 30, 2017 at 02:03:13PM +0100, Ulf Hansson wrote:
> On 30 October 2017 at 13:15, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > On Mon, Oct 30, 2017 at 12:40:39PM +0100, Ulf Hansson wrote:
> >> On 27 October 2017 at 21:31, SF Markus Elfring
> >> <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >> > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> >> > Date: Fri, 27 Oct 2017 21:21:40 +0200
> >> >
> >> > Add a jump target so that a specific string copy operation is stored
> >> > only once at the end of this function implementation.
> >> > Replace two calls of the function "strncpy" by goto statements.
> >> >
> >> > This issue was detected by using the Coccinelle software.
> >> >
> >> > Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> >>
> >> Thanks, applied for next!
> >>
> >
> > What's the advantage of this patch?  The new code seems more complicated
> > to me and GCC automatically reuses duplicate constant strings so there
> > is no memory savings.
> 
> It looked to me that the error path got a bit cleaner. However, I
> guess it's matter of taste.
> 
> If you insist, I can drop it.

I'm on the kernel-janitors list so I am CC'd on all of Markus's patches.
It's not my code and I'm tired of being the anti-Markus guy so this
patch is fine.  Markus has a tool that finds duplicate strings and he
uses gotos to avoid them.  I don't think duplicate strings are a problem
or that it's a good idea to send over a hundred patches using this
method.  But many people have explained that to Markus already and
that's not the bigger picture which is about error handling and labels.

What I like are labels that are necessary and self explanatory.  Things
like "goto unlock" are a good example, because we know we need to unlock
and the goto tells us what the label does.  Or here is another example:

	foo = alloc_foo();
	if (!foo)
		return -ENOMEM;

	bar = alloc_bar();
	if (!foo) {
		err = -ENOMEM;
		goto free_foo;
	}

>From reading the code, you know that you have to free foo and the label
name is clear so you literally don't need to scroll down to the bottom
of the function when you're reading this code.  A bad label is like
this:

	foo = alloc_foo();
	if (!foo)
		goto err;

You're reading along and you're like "what happens at the err" label?
So then you have to scroll down to the bottom and read the code, then
you have to think about how the variables line up with the variables
in the above code, then you have to scroll back and find your place
again and by that point you've forgotten what you were doing when you
started.

regards,
dan carpenter