[alsa-devel] [PATCH] ALSA: pcsp: Use common error handling code in snd_card_pcsp_probe()
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; }
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
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
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
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
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
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
- 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
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
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)
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
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)
participants (3)
-
Dan Carpenter -
SF Markus Elfring -
Takashi Iwai