[PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 22 Aug 2017 14:00:21 +0200

Add a jump target so that a bit of exception handling can be better reused
in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/drivers/pcsp/pcsp.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 72e2d0012084..c8ea4fed3905 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -108,22 +108,17 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		return err;
 
 	err = snd_pcsp_create(card);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err)
+		goto free_card;
+
 	if (!nopcm) {
 		err = snd_pcsp_new_pcm(&pcsp_chip);
-		if (err < 0) {
-			snd_card_free(card);
-			return err;
-		}
+		if (err)
+			goto free_card;
 	}
 	err = snd_pcsp_new_mixer(&pcsp_chip, nopcm);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err)
+		goto free_card;
 
 	strcpy(card->driver, "PC-Speaker");
 	strcpy(card->shortname, "pcsp");
@@ -131,7 +126,8 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		pcsp_chip.port);
 
 	err = snd_card_register(card);
-	if (err < 0) {
+	if (err) {
+free_card:
 		snd_card_free(card);
 		return err;
 	}
-- 
2.14.0

Re: [PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Dan Carpenter]

  	err = snd_card_register(card);
> -	if (err < 0) {
> +	if (err) {
> +free_card:
>  		snd_card_free(card);
>  		return err;
>  	}

I thought we agreed, yesterday, to never use this style of error
handling?

regards,
dan carpenter

Re: [PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 14:16:25 +0200,
Dan Carpenter wrote:
> 
>   	err = snd_card_register(card);
> > -	if (err < 0) {
> > +	if (err) {
> > +free_card:
> >  		snd_card_free(card);
> >  		return err;
> >  	}
> 
> I thought we agreed, yesterday, to never use this style of error
> handling?

Yeah, this is definitely no-go.

Also, please don't omit the negative value check.


thanks,

Takashi

Re: ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

> Also, please don't omit the negative value check.

Is it appropriate to treat non-zero values as error codes there generally?

Regards,
Markus

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 14:47:20 +0200,
SF Markus Elfring wrote:
> 
> > Also, please don't omit the negative value check.
> 
> Is it appropriate to treat non-zero values as error codes there generally?

No, it can't be in general.  Lots of functions return a positive
value, too.


Takashi

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

>> Is it appropriate to treat non-zero values as error codes there generally?
> 
> No, it can't be in general.

I got the impression that the functions which are called at the updated places
by the function “snd_card_pcsp_probe” indicate a successful execution
only by zero so far.


> Lots of functions return a positive value, too.

Would you like to point any example out from the programming interface?

Regards,
Markus

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 15:15:02 +0200,
SF Markus Elfring wrote:
> 
> >> Is it appropriate to treat non-zero values as error codes there generally?
> > 
> > No, it can't be in general.
> 
> I got the impression that the functions which are called at the updated places
> by the function “snd_card_pcsp_probe” indicate a successful execution
> only by zero so far.

You have the impression, great.  And what's the reason to drop the
negative check?  It's not clearer, not better readable.
And, the worst part is that you've done it silently even without
mentioning in the change log at all.  That's really bad.
Just don't do it.

> > Lots of functions return a positive value, too.
> 
> Would you like to point any example out from the programming interface?

For example, the control API functions may return the positive number
when the value got changed, 0 for else, and a negative number for the
error.  The functions returning some numbers may return positive
numbers, of course.


Takashi

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

>> I got the impression that the functions which are called at the updated places
>> by the function “snd_card_pcsp_probe” indicate a successful execution
>> only by zero so far.
> 
> You have the impression, great.

This aspect is also a general programming interface issue for some functions.


> And what's the reason to drop the negative check?

* I find it a bit safer when the error predicate is “return value != 0”.

* It is also a small source code reduction.


> It's not clearer, not better readable.

It seems that we have got different development opinions this time.


> And, the worst part is that you've done it silently even without
> mentioning in the change log at all.  That's really bad.
> Just don't do it.

I found it not relevant enough for the commit message.


> For example, the control API functions may return the positive number
> when the value got changed, 0 for else, and a negative number for the
> error.  The functions returning some numbers may return positive
> numbers, of course.

Did I touch any specific function calls which belong to this
programming interface category?

Regards,
Markus

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 16:36:35 +0200,
SF Markus Elfring wrote:
> 
> >> I got the impression that the functions which are called at the updated places
> >> by the function “snd_card_pcsp_probe” indicate a successful execution
> >> only by zero so far.
> > 
> > You have the impression, great.
> 
> This aspect is also a general programming interface issue for some functions.
> 
> 
> > And what's the reason to drop the negative check?
> 
> * I find it a bit safer when the error predicate is “return value != 0”.

Can't agree.  And I have no interest to continue bike-shedding,
sorry.  You can't convince me regarding this.


Takashi

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

>> * I find it a bit safer when the error predicate is “return value != 0”.
> 
> Can't agree.

How do you think about to reduce the probability that positive return values
will accidentally be interpreted as a successful function execution.


> And I have no interest to continue bike-shedding, sorry.

I do not like that you prefer to put this technical detail into such
a communication category.


> You can't convince me regarding this.

Would you still like to integrate the proposed refactoring with the use
of previous failure predicates then?

Regards,
Markus

Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 17:03:00 +0200,
SF Markus Elfring wrote:
> 
> >> * I find it a bit safer when the error predicate is “return value != 0”.
> > 
> > Can't agree.
> 
> How do you think about to reduce the probability that positive return values
> will accidentally be interpreted as a successful function execution.

It's not zero.

> > And I have no interest to continue bike-shedding, sorry.
> 
> I do not like that you prefer to put this technical detail into such
> a communication category.
> 
> 
> > You can't convince me regarding this.
> 
> Would you still like to integrate the proposed refactoring with the use
> of previous failure predicates then?

That's fine.

But, please don't forget what others already mentioned.
For example, Joe Perches suggested to put a blank line before the
label for your patches.  But you completely ignored it and did the
same again.


thanks,

Takashi

[PATCH v3] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 22 Aug 2017 17:33:33 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v3:
* An approach to make a few checks for a failure predicate a bit safer
  was rejected today.

* An extra blank line was added before a label.

v2:
Two statements were moved together with an additional jump label to the end
of this function.

 sound/drivers/pcsp/pcsp.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 72e2d0012084..0dd3f46eb03e 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -108,22 +108,17 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		return err;
 
 	err = snd_pcsp_create(card);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err < 0)
+		goto free_card;
+
 	if (!nopcm) {
 		err = snd_pcsp_new_pcm(&pcsp_chip);
-		if (err < 0) {
-			snd_card_free(card);
-			return err;
-		}
+		if (err < 0)
+			goto free_card;
 	}
 	err = snd_pcsp_new_mixer(&pcsp_chip, nopcm);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err < 0)
+		goto free_card;
 
 	strcpy(card->driver, "PC-Speaker");
 	strcpy(card->shortname, "pcsp");
@@ -131,12 +126,14 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		pcsp_chip.port);
 
 	err = snd_card_register(card);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err < 0)
+		goto free_card;
 
 	return 0;
+
+free_card:
+	snd_card_free(card);
+	return err;
 }
 
 static int alsa_card_pcsp_init(struct device *dev)
-- 
2.14.0

Re: [PATCH v3] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[Takashi Iwai]

On Tue, 22 Aug 2017 17:47:26 +0200,
SF Markus Elfring wrote:
> 
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 22 Aug 2017 17:33:33 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied, thanks.


Takashi

[PATCH v2] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 22 Aug 2017 15:50:53 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
Do you find this refactoring acceptable instead?

 sound/drivers/pcsp/pcsp.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 72e2d0012084..fc83139fcfa2 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -108,22 +108,17 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		return err;
 
 	err = snd_pcsp_create(card);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err)
+		goto free_card;
+
 	if (!nopcm) {
 		err = snd_pcsp_new_pcm(&pcsp_chip);
-		if (err < 0) {
-			snd_card_free(card);
-			return err;
-		}
+		if (err)
+			goto free_card;
 	}
 	err = snd_pcsp_new_mixer(&pcsp_chip, nopcm);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err)
+		goto free_card;
 
 	strcpy(card->driver, "PC-Speaker");
 	strcpy(card->shortname, "pcsp");
@@ -131,12 +126,13 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 		pcsp_chip.port);
 
 	err = snd_card_register(card);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err)
+		goto free_card;
 
 	return 0;
+free_card:
+	snd_card_free(card);
+	return err;
 }
 
 static int alsa_card_pcsp_init(struct device *dev)
-- 
2.14.0