Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1717353 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-22 14:20 +0200
    Re: [PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 14:30 +0200
      Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 15:00 +0200
        Re: [alsa-devel] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 15:20 +0200
          Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 16:10 +0200
            Re: [alsa-devel] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 16:40 +0200
              Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 16:50 +0200
                Re: [alsa-devel] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 17:10 +0200
                  [PATCH v3] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 17:50 +0200
                    Re: [PATCH v3] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe() Takashi Iwai <tiwai@suse.de> - 2017-08-22 18:10 +0200
                Re: [alsa-devel] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 17:10 +0200
      Re: ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 15:00 +0200
      [PATCH v2] ALSA: pcsp: Use common error handling code in  snd_card_pcsp_probe() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-22 16:10 +0200

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

  	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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web