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

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

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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@users.sourceforge.net>
---
 drivers/mmc/host/vub300.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index c1a169843f99..c898a45a84c5 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -1244,12 +1244,8 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
 						USB_RECIP_DEVICE, 0x0000, 0x0000,
 						xfer_buffer, xfer_length, HZ);
 			kfree(xfer_buffer);
-			if (retval < 0) {
-				strncpy(vub300->vub_name,
-					"SDIO pseudocode download failed",
-					sizeof(vub300->vub_name));
-				return;
-			}
+			if (retval < 0)
+				goto copy_error_message;
 		} else {
 			dev_err(&vub300->udev->dev,
 				"not enough memory for xfer buffer to send"
@@ -1291,12 +1287,8 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
 						USB_RECIP_DEVICE, 0x0000, 0x0000,
 						xfer_buffer, xfer_length, HZ);
 			kfree(xfer_buffer);
-			if (retval < 0) {
-				strncpy(vub300->vub_name,
-					"SDIO pseudocode download failed",
-					sizeof(vub300->vub_name));
-				return;
-			}
+			if (retval < 0)
+				goto copy_error_message;
 		} else {
 			dev_err(&vub300->udev->dev,
 				"not enough memory for xfer buffer to send"
@@ -1349,6 +1341,12 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
 			sizeof(vub300->vub_name));
 		return;
 	}
+
+	return;
+
+copy_error_message:
+	strncpy(vub300->vub_name, "SDIO pseudocode download failed",
+		sizeof(vub300->vub_name));
 }
 
 /*
-- 
2.14.3

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

[Ulf Hansson]

On 27 October 2017 at 21:31, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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@users.sourceforge.net>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/vub300.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> index c1a169843f99..c898a45a84c5 100644
> --- a/drivers/mmc/host/vub300.c
> +++ b/drivers/mmc/host/vub300.c
> @@ -1244,12 +1244,8 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
>                                                 USB_RECIP_DEVICE, 0x0000, 0x0000,
>                                                 xfer_buffer, xfer_length, HZ);
>                         kfree(xfer_buffer);
> -                       if (retval < 0) {
> -                               strncpy(vub300->vub_name,
> -                                       "SDIO pseudocode download failed",
> -                                       sizeof(vub300->vub_name));
> -                               return;
> -                       }
> +                       if (retval < 0)
> +                               goto copy_error_message;
>                 } else {
>                         dev_err(&vub300->udev->dev,
>                                 "not enough memory for xfer buffer to send"
> @@ -1291,12 +1287,8 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
>                                                 USB_RECIP_DEVICE, 0x0000, 0x0000,
>                                                 xfer_buffer, xfer_length, HZ);
>                         kfree(xfer_buffer);
> -                       if (retval < 0) {
> -                               strncpy(vub300->vub_name,
> -                                       "SDIO pseudocode download failed",
> -                                       sizeof(vub300->vub_name));
> -                               return;
> -                       }
> +                       if (retval < 0)
> +                               goto copy_error_message;
>                 } else {
>                         dev_err(&vub300->udev->dev,
>                                 "not enough memory for xfer buffer to send"
> @@ -1349,6 +1341,12 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300,
>                         sizeof(vub300->vub_name));
>                 return;
>         }
> +
> +       return;
> +
> +copy_error_message:
> +       strncpy(vub300->vub_name, "SDIO pseudocode download failed",
> +               sizeof(vub300->vub_name));
>  }
>
>  /*
> --
> 2.14.3
>

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

[Dan Carpenter]

On Mon, Oct 30, 2017 at 12:40:39PM +0100, Ulf Hansson wrote:
> On 27 October 2017 at 21:31, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > 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@users.sourceforge.net>
> 
> 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.

regards,
dan capenter

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

[Ulf Hansson]

On 30 October 2017 at 13:15, Dan Carpenter <dan.carpenter@oracle.com> 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@users.sourceforge.net> wrote:
>> > From: Markus Elfring <elfring@users.sourceforge.net>
>> > 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@users.sourceforge.net>
>>
>> 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.

Kind regards
Uffe

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@oracle.com> 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@users.sourceforge.net> wrote:
> >> > From: Markus Elfring <elfring@users.sourceforge.net>
> >> > 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@users.sourceforge.net>
> >>
> >> 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

Re: mmc: vub300: Use common code in __download_offload_pseudocode()

[SF Markus Elfring]

>>> 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.

Do you want that I omit this address from the list of recipients?


> It's not my code and I'm tired of being the anti-Markus guy

Interesting …

But I got the impression that this special relationship resulted
also in a few useful side effects.


> so this patch is fine.

Thanks for another bit of acceptance.


> Markus has a tool that finds duplicate strings and he uses gotos
> to avoid them.

Yes. - The script which I am using for the semantic patch language
(Coccinelle software) can not only find this implementation detail
but also duplicate source code generally to some degree.


> I don't think duplicate strings are a problem

They can become an issue for further considerations if inappropriate
error messages were used for example.

There are more statement combinations which can be improved a bit more.


> or that it's a good idea to send over a hundred patches using this method.

The change acceptance is varying as usual.


> But many people have explained that to Markus already

I hope that my contributions can improve the affected software in some areas.


> 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.

It seems that this is a topic where you got strong opinions about.


> You're reading along and you're like "what happens at the err" label?

Would you accept any further adjustments around questionable jump targets?

Regards,
Markus

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

[Ulf Hansson]

On 31 October 2017 at 09:45, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Oct 30, 2017 at 02:03:13PM +0100, Ulf Hansson wrote:
>> On 30 October 2017 at 13:15, Dan Carpenter <dan.carpenter@oracle.com> 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@users.sourceforge.net> wrote:
>> >> > From: Markus Elfring <elfring@users.sourceforge.net>
>> >> > 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@users.sourceforge.net>
>> >>
>> >> 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.

Thanks for the details! Much appreciated!

Regarding posts from Markus' I am well aware of the problems.

In my case, it hasn't been much of a hurdle, I simply ignore changes
that seems silly to me.

Kind regards
Uffe

Re: mmc: vub300: Use common code in __download_offload_pseudocode()

[SF Markus Elfring]

> Regarding posts from Markus' I am well aware of the problems.

Interesting …


> In my case, it hasn't been much of a hurdle, I simply ignore changes
> that seems silly to me.

* How often did this happen so far?

* Are any update suggestions left over where a constructive feedback
  will make sense?

Regards,
Markus