* Re: [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params()
2016-08-21 19:45 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() SF Markus Elfring
@ 2016-08-21 19:51 ` Joe Perches
2016-08-22 8:34 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() SF Markus Elfring
2016-08-21 20:36 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() Julia Lawall
2016-08-22 7:20 ` walter harms
2 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2016-08-21 19:51 UTC (permalink / raw)
To: SF Markus Elfring, alsa-devel, Jaroslav Kysela, Takashi Iwai,
Vinod Koul
Cc: LKML, kernel-janitors, Julia Lawall
On Sun, 2016-08-21 at 21:45 +0200, SF Markus Elfring wrote:
> Reduce the scope for the local variables to an if branch.
[]
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
[]
> @@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params)
> static int
> snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> {
> - struct snd_compr_params *params;
> - int retval;
> -
> if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
Likely better not reducing variable scope but changing:
if (stream->runtime->state == SNDRV_PCM_STATE_OPEN)
to
if (stream->runtime->state != SNDRV_PCM_STATE_OPEN)
return -EPERM;
and unindenting the remainder of the code one level instead.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params()
2016-08-21 19:51 ` Joe Perches
@ 2016-08-22 8:34 ` SF Markus Elfring
2016-08-22 8:38 ` [PATCH v2 1/2] ALSA: compress: Restructure source code around an if statement in snd_compr_set_params() SF Markus Elfring
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: SF Markus Elfring @ 2016-08-22 8:34 UTC (permalink / raw)
To: alsa-devel, Jaroslav Kysela, Joe Perches, Takashi Iwai,
Vinod Koul
Cc: LKML, kernel-janitors, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 22 Aug 2016 10:27:01 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Restructure source code around an if statement
Use memdup_user()
sound/core/compress_offload.c | 58 +++++++++++++++++++------------------------
1 file changed, 26 insertions(+), 32 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2 1/2] ALSA: compress: Restructure source code around an if statement in snd_compr_set_params()
2016-08-22 8:34 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() SF Markus Elfring
@ 2016-08-22 8:38 ` SF Markus Elfring
2016-08-22 8:40 ` [PATCH v2 2/2] ALSA: compress: Use memdup_user() rather than duplicating its implementation SF Markus Elfring
2016-08-23 4:04 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() Vinod Koul
2 siblings, 0 replies; 14+ messages in thread
From: SF Markus Elfring @ 2016-08-22 8:38 UTC (permalink / raw)
To: alsa-devel, Jaroslav Kysela, Joe Perches, Takashi Iwai,
Vinod Koul
Cc: LKML, kernel-janitors, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 22 Aug 2016 10:01:52 +0200
* Reverse a condition check.
* Reduce the indentation one level then for some source code
from a previous if branch.
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
sound/core/compress_offload.c | 62 +++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 2c49848..a10d139 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -548,43 +548,41 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
struct snd_compr_params *params;
int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
- /*
- * we should allow parameter change only when stream has been
- * opened not in other cases
- */
- params = kmalloc(sizeof(*params), GFP_KERNEL);
- if (!params)
- return -ENOMEM;
- if (copy_from_user(params, (void __user *)arg, sizeof(*params))) {
- retval = -EFAULT;
- goto out;
- }
+ if (stream->runtime->state != SNDRV_PCM_STATE_OPEN)
+ return -EPERM;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ params = kmalloc(sizeof(*params), GFP_KERNEL);
+ if (!params)
+ return -ENOMEM;
+ if (copy_from_user(params, (void __user *)arg, sizeof(*params))) {
+ retval = -EFAULT;
+ goto out;
+ }
- retval = snd_compress_check_input(params);
- if (retval)
- goto out;
+ retval = snd_compress_check_input(params);
+ if (retval)
+ goto out;
- retval = snd_compr_allocate_buffer(stream, params);
- if (retval) {
- retval = -ENOMEM;
- goto out;
- }
+ retval = snd_compr_allocate_buffer(stream, params);
+ if (retval) {
+ retval = -ENOMEM;
+ goto out;
+ }
- retval = stream->ops->set_params(stream, params);
- if (retval)
- goto out;
+ retval = stream->ops->set_params(stream, params);
+ if (retval)
+ goto out;
- stream->metadata_set = false;
- stream->next_track = false;
+ stream->metadata_set = false;
+ stream->next_track = false;
- if (stream->direction == SND_COMPRESS_PLAYBACK)
- stream->runtime->state = SNDRV_PCM_STATE_SETUP;
- else
- stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
- } else {
- return -EPERM;
- }
+ if (stream->direction == SND_COMPRESS_PLAYBACK)
+ stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+ else
+ stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
out:
kfree(params);
return retval;
--
2.9.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/2] ALSA: compress: Use memdup_user() rather than duplicating its implementation
2016-08-22 8:34 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() SF Markus Elfring
2016-08-22 8:38 ` [PATCH v2 1/2] ALSA: compress: Restructure source code around an if statement in snd_compr_set_params() SF Markus Elfring
@ 2016-08-22 8:40 ` SF Markus Elfring
2016-08-23 4:04 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() Vinod Koul
2 siblings, 0 replies; 14+ messages in thread
From: SF Markus Elfring @ 2016-08-22 8:40 UTC (permalink / raw)
To: alsa-devel, Jaroslav Kysela, Joe Perches, Takashi Iwai,
Vinod Koul
Cc: LKML, kernel-janitors, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 22 Aug 2016 10:12:37 +0200
Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
sound/core/compress_offload.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a10d139..786989b 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -554,13 +554,9 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
* we should allow parameter change only when stream has been
* opened not in other cases
*/
- params = kmalloc(sizeof(*params), GFP_KERNEL);
- if (!params)
- return -ENOMEM;
- if (copy_from_user(params, (void __user *)arg, sizeof(*params))) {
- retval = -EFAULT;
- goto out;
- }
+ params = memdup_user((void __user *)arg, sizeof(*params));
+ if (IS_ERR(params))
+ return PTR_ERR(params);
retval = snd_compress_check_input(params);
if (retval)
--
2.9.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params()
2016-08-22 8:34 ` [PATCH v2 0/2] ALSA: compress: Fine-tuning for snd_compr_set_params() SF Markus Elfring
2016-08-22 8:38 ` [PATCH v2 1/2] ALSA: compress: Restructure source code around an if statement in snd_compr_set_params() SF Markus Elfring
2016-08-22 8:40 ` [PATCH v2 2/2] ALSA: compress: Use memdup_user() rather than duplicating its implementation SF Markus Elfring
@ 2016-08-23 4:04 ` Vinod Koul
2 siblings, 0 replies; 14+ messages in thread
From: Vinod Koul @ 2016-08-23 4:04 UTC (permalink / raw)
To: SF Markus Elfring
Cc: alsa-devel, Jaroslav Kysela, Joe Perches, Takashi Iwai, LKML,
kernel-janitors, Julia Lawall
On Mon, Aug 22, 2016 at 10:34:19AM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 22 Aug 2016 10:27:01 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
Both:
Acked-by: Vinod Koul <vinod.koul@intel.com>
--
~Vinod
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params()
2016-08-21 19:45 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() SF Markus Elfring
2016-08-21 19:51 ` Joe Perches
@ 2016-08-21 20:36 ` Julia Lawall
2016-08-22 5:01 ` Vinod Koul
2016-08-22 7:20 ` walter harms
2 siblings, 1 reply; 14+ messages in thread
From: Julia Lawall @ 2016-08-21 20:36 UTC (permalink / raw)
To: SF Markus Elfring
Cc: alsa-devel, Jaroslav Kysela, Takashi Iwai, Vinod Koul, LKML,
kernel-janitors
On Sun, 21 Aug 2016, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 21 Aug 2016 21:26:18 +0200
>
> Reduce the scope for the local variables to an if branch.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> sound/core/compress_offload.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 583d407..b43aec5 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params)
> static int
> snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> {
> - struct snd_compr_params *params;
> - int retval;
> -
> if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
> /*
> * we should allow parameter change only when stream has been
> * opened not in other cases
> */
> + int retval;
> + struct snd_compr_params *params;
I don't like this at all. Local variables should be at the top of the
function, not hiding under 4 lines of comments in the middle of the code.
julia
> +
> params = memdup_user((void __user *)arg, sizeof(*params));
> if (IS_ERR(params))
> return PTR_ERR(params);
> @@ -578,12 +578,12 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> else
> stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
> +out:
> + kfree(params);
> + return retval;
> } else {
> return -EPERM;
> }
> -out:
> - kfree(params);
> - return retval;
> }
>
> static int
> --
> 2.9.3
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params()
2016-08-21 20:36 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() Julia Lawall
@ 2016-08-22 5:01 ` Vinod Koul
0 siblings, 0 replies; 14+ messages in thread
From: Vinod Koul @ 2016-08-22 5:01 UTC (permalink / raw)
To: Julia Lawall
Cc: alsa-devel, kernel-janitors, LKML, Takashi Iwai,
SF Markus Elfring
On Sun, Aug 21, 2016 at 04:36:22PM -0400, Julia Lawall wrote:
>
>
> On Sun, 21 Aug 2016, SF Markus Elfring wrote:
>
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 21 Aug 2016 21:26:18 +0200
> >
> > Reduce the scope for the local variables to an if branch.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> > sound/core/compress_offload.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index 583d407..b43aec5 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params)
> > static int
> > snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> > {
> > - struct snd_compr_params *params;
> > - int retval;
> > -
> > if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
> > /*
> > * we should allow parameter change only when stream has been
> > * opened not in other cases
> > */
> > + int retval;
> > + struct snd_compr_params *params;
>
> I don't like this at all. Local variables should be at the top of the
> function, not hiding under 4 lines of comments in the middle of the code.
I agree with you this, it doesn't help IMO as well
--
~Vinod
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params()
2016-08-21 19:45 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() SF Markus Elfring
2016-08-21 19:51 ` Joe Perches
2016-08-21 20:36 ` [PATCH 2/2] ALSA: compress: Reduce the scope for two variables in snd_compr_set_params() Julia Lawall
@ 2016-08-22 7:20 ` walter harms
2 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2016-08-22 7:20 UTC (permalink / raw)
To: SF Markus Elfring
Cc: alsa-devel, Jaroslav Kysela, Takashi Iwai, Vinod Koul, LKML,
kernel-janitors, Julia Lawall
Am 21.08.2016 21:45, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 21 Aug 2016 21:26:18 +0200
>
> Reduce the scope for the local variables to an if branch.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> sound/core/compress_offload.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 583d407..b43aec5 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -545,14 +545,14 @@ static int snd_compress_check_input(struct snd_compr_params *params)
> static int
> snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> {
> - struct snd_compr_params *params;
> - int retval;
> -
> if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
> /*
> * we should allow parameter change only when stream has been
> * opened not in other cases
> */
> + int retval;
> + struct snd_compr_params *params;
> +
> params = memdup_user((void __user *)arg, sizeof(*params));
> if (IS_ERR(params))
> return PTR_ERR(params);
> @@ -578,12 +578,12 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> else
> stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
> +out:
> + kfree(params);
> + return retval;
> } else {
> return -EPERM;
> }
> -out:
> - kfree(params);
> - return retval;
> }
>
> static int
if would make sense to have
if (stream->runtime->state != SNDRV_PCM_STATE_OPEN)
return -EPERM;
and read adjust the indent.
just my 2 cents
re,
wh
^ permalink raw reply [flat|nested] 14+ messages in thread