[alsa-devel] [PATCH 0/2] ALSA: ctxfi: Delete an unnecessary check before kfree()
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 2 Dec 2014 20:35:41 +0100
Another update suggestion was taken into account after a patch was applied from static source code analysis.
Markus Elfring (2): Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
sound/pci/ctxfi/ctdaio.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 2 Dec 2014 20:00:33 +0100
The kfree() function performs also input parameter validation. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/pci/ctxfi/ctdaio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index c1c3f88..1712332 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -577,7 +577,7 @@ static int get_daio_rsc(struct daio_mgr *mgr, error: if (dao) kfree(dao); - else if (dai) + else kfree(dai);
spin_lock_irqsave(&mgr->mgr_lock, flags);
On Tue, 2014-12-02 at 21:55 +0100, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 2 Dec 2014 20:00:33 +0100
The kfree() function performs also input parameter validation. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
sound/pci/ctxfi/ctdaio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index c1c3f88..1712332 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -577,7 +577,7 @@ static int get_daio_rsc(struct daio_mgr *mgr, error: if (dao) kfree(dao);
- else if (dai)
else kfree(dai);
spin_lock_irqsave(&mgr->mgr_lock, flags);
I think this not nice and would prefer something like:
--- sound/pci/ctxfi/ctdaio.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index c1c3f88..9b87dd2 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -528,8 +528,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, struct daio **rdaio) { int err; - struct dai *dai = NULL; - struct dao *dao = NULL; unsigned long flags;
*rdaio = NULL; @@ -544,27 +542,30 @@ static int get_daio_rsc(struct daio_mgr *mgr, return err; }
+ err = -ENOMEM; /* Allocate mem for daio resource */ if (desc->type <= DAIO_OUT_MAX) { - dao = kzalloc(sizeof(*dao), GFP_KERNEL); - if (!dao) { - err = -ENOMEM; + struct dao *dao = kzalloc(sizeof(*dao), GFP_KERNEL); + if (!dao) goto error; - } + err = dao_rsc_init(dao, desc, mgr); - if (err) + if (err) { + kfree(dao); goto error; + }
*rdaio = &dao->daio; } else { - dai = kzalloc(sizeof(*dai), GFP_KERNEL); - if (!dai) { - err = -ENOMEM; + struct dai *dai = kzalloc(sizeof(*dai), GFP_KERNEL); + if (!dai) goto error; - } + err = dai_rsc_init(dai, desc, mgr); - if (err) + if (err) { + kfree(dai); goto error; + }
*rdaio = &dai->daio; } @@ -575,11 +576,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, return 0;
error: - if (dao) - kfree(dao); - else if (dai) - kfree(dai); - spin_lock_irqsave(&mgr->mgr_lock, flags); daio_mgr_put_rsc(&mgr->mgr, desc->type); spin_unlock_irqrestore(&mgr->mgr_lock, flags);
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 2 Dec 2014 20:33:51 +0100
The kfree() function was called in two cases by the get_daio_rsc() function during error handling even if the passed variable contained still a null pointer. This implementation detail could be improved by adjustments for jump labels.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/pci/ctxfi/ctdaio.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index 1712332..a12489e 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -549,22 +549,22 @@ static int get_daio_rsc(struct daio_mgr *mgr, dao = kzalloc(sizeof(*dao), GFP_KERNEL); if (!dao) { err = -ENOMEM; - goto error; + goto alloc_error; } err = dao_rsc_init(dao, desc, mgr); if (err) - goto error; + goto dao_init_error;
*rdaio = &dao->daio; } else { dai = kzalloc(sizeof(*dai), GFP_KERNEL); if (!dai) { err = -ENOMEM; - goto error; + goto alloc_error; } err = dai_rsc_init(dai, desc, mgr); if (err) - goto error; + goto dai_init_error;
*rdaio = &dai->daio; } @@ -574,12 +574,12 @@ static int get_daio_rsc(struct daio_mgr *mgr,
return 0;
-error: - if (dao) - kfree(dao); - else - kfree(dai); - +dao_init_error: + kfree(dao); + goto alloc_error; +dai_init_error: + kfree(dai); +alloc_error: spin_lock_irqsave(&mgr->mgr_lock, flags); daio_mgr_put_rsc(&mgr->mgr, desc->type); spin_unlock_irqrestore(&mgr->mgr_lock, flags);
At Tue, 02 Dec 2014 21:50:26 +0100, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Tue, 2 Dec 2014 20:35:41 +0100
Another update suggestion was taken into account after a patch was applied from static source code analysis.
Markus Elfring (2): Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
In these cases, the changes aren't so straightforward, and they don't improve the readability.
Takashi
Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
In these cases, the changes aren't so straightforward, and they don't improve the readability.
How do you prefer to improve the affected source code here?
Regards, Markus
At Wed, 03 Dec 2014 12:38:51 +0100, SF Markus Elfring wrote:
Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
In these cases, the changes aren't so straightforward, and they don't improve the readability.
How do you prefer to improve the affected source code here?
Don't touch then. There are no bugs to fix there.
Takashi
On Wed, 2014-12-03 at 13:41 +0100, Takashi Iwai wrote:
At Wed, 03 Dec 2014 12:38:51 +0100, SF Markus Elfring wrote:
Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
In these cases, the changes aren't so straightforward, and they don't improve the readability.
How do you prefer to improve the affected source code here?
Don't touch then. There are no bugs to fix there.
Takashi, what did you think of this? https://lkml.org/lkml/2014/12/2/771
Just unnecessary?
At Wed, 03 Dec 2014 09:14:48 -0800, Joe Perches wrote:
On Wed, 2014-12-03 at 13:41 +0100, Takashi Iwai wrote:
At Wed, 03 Dec 2014 12:38:51 +0100, SF Markus Elfring wrote:
Deletion of an unnecessary check before the function call "kfree" One function call less in get_daio_rsc() after error detection
In these cases, the changes aren't so straightforward, and they don't improve the readability.
How do you prefer to improve the affected source code here?
Don't touch then. There are no bugs to fix there.
Takashi, what did you think of this? https://lkml.org/lkml/2014/12/2/771
Just unnecessary?
Well, this one looks more consistent. But honestly speaking, it's rather a matter of taste. So I'm not so much inclined to merge the stuff, too, sorry. If it's proven to reduce the compiled size, etc, I'll happily apply it, though.
FWIW, what wasn't good in the original patch was to break the balance. It removed only the check for dai, and not for dao. One would wonder why there is a check only for one.
It could be two simple kfree() calls instead. But then this won't be an improvement, as it gets one more function call, which is more expensive than a conditional.
Takashi
Move the pointer declarations into the blocks that use them. Neaten the kfree calls when the _init functions fail.
Trivially reduces object size (defconfig x86-64)
$ size sound/pci/ctxfi/ctdaio.o.* text data bss dec hex filename 5287 224 0 5511 1587 sound/pci/ctxfi/ctdaio.o.new 5319 224 0 5543 15a7 sound/pci/ctxfi/ctdaio.o.old
Signed-off-by: Joe Perches joe@perches.com Noticed-by: Markus Elfring elfring@users.sourceforge.net --- On Wed, 2014-12-03 at 18:30 +0100, Takashi Iwai wrote:
At Wed, 03 Dec 2014 09:14:48 -0800, Joe Perches wrote:
On Wed, 2014-12-03 at 13:41 +0100, Takashi Iwai wrote: Takashi, what did you think of this? https://lkml.org/lkml/2014/12/2/771
Just unnecessary?
Well, this one looks more consistent. But honestly speaking, it's rather a matter of taste. So I'm not so much inclined to merge the stuff, too, sorry. If it's proven to reduce the compiled size, etc, I'll happily apply it, though.
sound/pci/ctxfi/ctdaio.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index c1c3f88..9b87dd2 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -528,8 +528,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, struct daio **rdaio) { int err; - struct dai *dai = NULL; - struct dao *dao = NULL; unsigned long flags;
*rdaio = NULL; @@ -544,27 +542,30 @@ static int get_daio_rsc(struct daio_mgr *mgr, return err; }
+ err = -ENOMEM; /* Allocate mem for daio resource */ if (desc->type <= DAIO_OUT_MAX) { - dao = kzalloc(sizeof(*dao), GFP_KERNEL); - if (!dao) { - err = -ENOMEM; + struct dao *dao = kzalloc(sizeof(*dao), GFP_KERNEL); + if (!dao) goto error; - } + err = dao_rsc_init(dao, desc, mgr); - if (err) + if (err) { + kfree(dao); goto error; + }
*rdaio = &dao->daio; } else { - dai = kzalloc(sizeof(*dai), GFP_KERNEL); - if (!dai) { - err = -ENOMEM; + struct dai *dai = kzalloc(sizeof(*dai), GFP_KERNEL); + if (!dai) goto error; - } + err = dai_rsc_init(dai, desc, mgr); - if (err) + if (err) { + kfree(dai); goto error; + }
*rdaio = &dai->daio; } @@ -575,11 +576,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, return 0;
error: - if (dao) - kfree(dao); - else if (dai) - kfree(dai); - spin_lock_irqsave(&mgr->mgr_lock, flags); daio_mgr_put_rsc(&mgr->mgr, desc->type); spin_unlock_irqrestore(&mgr->mgr_lock, flags);
At Wed, 03 Dec 2014 09:59:31 -0800, Joe Perches wrote:
Move the pointer declarations into the blocks that use them. Neaten the kfree calls when the _init functions fail.
Trivially reduces object size (defconfig x86-64)
$ size sound/pci/ctxfi/ctdaio.o.* text data bss dec hex filename 5287 224 0 5511 1587 sound/pci/ctxfi/ctdaio.o.new 5319 224 0 5543 15a7 sound/pci/ctxfi/ctdaio.o.old
Signed-off-by: Joe Perches joe@perches.com Noticed-by: Markus Elfring elfring@users.sourceforge.net
On Wed, 2014-12-03 at 18:30 +0100, Takashi Iwai wrote:
At Wed, 03 Dec 2014 09:14:48 -0800, Joe Perches wrote:
On Wed, 2014-12-03 at 13:41 +0100, Takashi Iwai wrote: Takashi, what did you think of this? https://lkml.org/lkml/2014/12/2/771
Just unnecessary?
Well, this one looks more consistent. But honestly speaking, it's rather a matter of taste. So I'm not so much inclined to merge the stuff, too, sorry. If it's proven to reduce the compiled size, etc, I'll happily apply it, though.
Thanks, applied now.
Takashi
sound/pci/ctxfi/ctdaio.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c index c1c3f88..9b87dd2 100644 --- a/sound/pci/ctxfi/ctdaio.c +++ b/sound/pci/ctxfi/ctdaio.c @@ -528,8 +528,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, struct daio **rdaio) { int err;
struct dai *dai = NULL;
struct dao *dao = NULL; unsigned long flags;
*rdaio = NULL;
@@ -544,27 +542,30 @@ static int get_daio_rsc(struct daio_mgr *mgr, return err; }
- err = -ENOMEM; /* Allocate mem for daio resource */ if (desc->type <= DAIO_OUT_MAX) {
dao = kzalloc(sizeof(*dao), GFP_KERNEL);if (!dao) {err = -ENOMEM;
struct dao *dao = kzalloc(sizeof(*dao), GFP_KERNEL);if (!dao) goto error;
}
- err = dao_rsc_init(dao, desc, mgr);
if (err)
if (err) {kfree(dao); goto error;}*rdaio = &dao->daio; } else {
dai = kzalloc(sizeof(*dai), GFP_KERNEL);if (!dai) {err = -ENOMEM;
struct dai *dai = kzalloc(sizeof(*dai), GFP_KERNEL);if (!dai) goto error;
}
- err = dai_rsc_init(dai, desc, mgr);
if (err)
if (err) {kfree(dai); goto error;}*rdaio = &dai->daio; }
@@ -575,11 +576,6 @@ static int get_daio_rsc(struct daio_mgr *mgr, return 0;
error:
- if (dao)
kfree(dao);- else if (dai)
kfree(dai);- spin_lock_irqsave(&mgr->mgr_lock, flags); daio_mgr_put_rsc(&mgr->mgr, desc->type); spin_unlock_irqrestore(&mgr->mgr_lock, flags);
participants (3)
-
Joe Perches -
SF Markus Elfring -
Takashi Iwai