[alsa-devel] [PATCH 0/7] ALSA: Fine-tuning for several function implementations
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 11:02:34 +0100 Some update suggestions were taken into account from static source code analysis. Markus Elfring (7): seq: Delete unnecessary checks in snd_seq_ioctl_query_subs() seq: Delete unnecessary checks in snd_seq_ioctl_get_subscription() seq: Delete unnecessary checks in snd_seq_ioctl_unsubscribe_port() seq: Delete unnecessary checks in snd_seq_ioctl_subscribe_port() seq: Adjust 16 function calls together with a variable assignment dmasound_core: Move two assignments for the variable "ret" in state_open() dmasound_core: Adjust six function calls together with a variable assignment sound/core/seq/seq_clientmgr.c | 163 ++++++++++++++++++++----------------- sound/oss/dmasound/dmasound_core.c | 33 +++++--- 2 files changed, 109 insertions(+), 87 deletions(-) -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 08:00:34 +0100 Two checks were repeated by the snd_seq_ioctl_query_subs() function despite of pointer determinations for the used variables at the beginning. * Adjust jump targets according to the Linux coding style convention. * Delete extra variable initialisations and the repeated checks which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/core/seq/seq_clientmgr.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 4c935202ce23..594c341a193e 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1925,16 +1925,16 @@ static int snd_seq_ioctl_query_subs(struct snd_seq_client *client, void *arg) { struct snd_seq_query_subs *subs = arg; int result = -ENXIO; - struct snd_seq_client *cptr = NULL; - struct snd_seq_client_port *port = NULL; + struct snd_seq_client *cptr; + struct snd_seq_client_port *port; struct snd_seq_port_subs_info *group; struct list_head *p; int i; if ((cptr = snd_seq_client_use_ptr(subs->root.client)) == NULL) - goto __end; + goto exit; if ((port = snd_seq_port_use_ptr(cptr, subs->root.port)) == NULL) - goto __end; + goto unlock_client; switch (subs->type) { case SNDRV_SEQ_QUERY_SUBS_READ: @@ -1944,7 +1944,7 @@ static int snd_seq_ioctl_query_subs(struct snd_seq_client *client, void *arg) group = &port->c_dest; break; default: - goto __end; + goto unlock_port; } down_read(&group->list_mutex); @@ -1970,13 +1970,11 @@ static int snd_seq_ioctl_query_subs(struct snd_seq_client *client, void *arg) } } up_read(&group->list_mutex); - - __end: - if (port) - snd_seq_port_unlock(port); - if (cptr) - snd_seq_client_unlock(cptr); - +unlock_port: + snd_seq_port_unlock(port); +unlock_client: + snd_seq_client_unlock(cptr); +exit: return result; } -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 08:54:36 +0100 Two checks were repeated by the snd_seq_ioctl_get_subscription() function despite of pointer determinations for the used variables at the beginning. * Adjust jump targets according to the Linux coding style convention. * Convert an assignment for the variable "result" to a direct initialisation. * Delete extra variable initialisations and the repeated checks which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/core/seq/seq_clientmgr.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 594c341a193e..8e804ff784ed 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1891,16 +1891,15 @@ static int snd_seq_ioctl_get_subscription(struct snd_seq_client *client, void *arg) { struct snd_seq_port_subscribe *subs = arg; - int result; - struct snd_seq_client *sender = NULL; - struct snd_seq_client_port *sport = NULL; + int result = -EINVAL; + struct snd_seq_client *sender; + struct snd_seq_client_port *sport; struct snd_seq_subscribers *p; - result = -EINVAL; if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) - goto __end; + goto exit; if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) - goto __end; + goto unlock_client; p = snd_seq_port_get_subscription(&sport->c_src, &subs->dest); if (p) { result = 0; @@ -1908,12 +1907,10 @@ static int snd_seq_ioctl_get_subscription(struct snd_seq_client *client, } else result = -ENOENT; - __end: - if (sport) - snd_seq_port_unlock(sport); - if (sender) - snd_seq_client_unlock(sender); - + snd_seq_port_unlock(sport); +unlock_client: + snd_seq_client_unlock(sender); +exit: return result; } -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 09:01:17 +0100 Four checks were repeated by the snd_seq_ioctl_unsubscribe_port() function despite of pointer determinations for the used variables at the beginning. * Adjust jump targets according to the Linux coding style convention. * Delete extra variable initialisations and the repeated checks which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/core/seq/seq_clientmgr.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 8e804ff784ed..ba26d622cb71 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1465,35 +1465,35 @@ static int snd_seq_ioctl_unsubscribe_port(struct snd_seq_client *client, { struct snd_seq_port_subscribe *subs = arg; int result = -ENXIO; - struct snd_seq_client *receiver = NULL, *sender = NULL; - struct snd_seq_client_port *sport = NULL, *dport = NULL; + struct snd_seq_client *receiver, *sender; + struct snd_seq_client_port *sport, *dport; if ((receiver = snd_seq_client_use_ptr(subs->dest.client)) == NULL) - goto __end; + goto exit; if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) - goto __end; + goto unlock_receiver; if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) - goto __end; + goto unlock_sender; if ((dport = snd_seq_port_use_ptr(receiver, subs->dest.port)) == NULL) - goto __end; + goto unlock_sport; result = check_subscription_permission(client, sport, dport, subs); if (result < 0) - goto __end; + goto unlock_dport; result = snd_seq_port_disconnect(client, sender, sport, receiver, dport, subs); if (! result) /* broadcast announce */ snd_seq_client_notify_subscription(SNDRV_SEQ_ADDRESS_SUBSCRIBERS, 0, subs, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED); - __end: - if (sport) - snd_seq_port_unlock(sport); - if (dport) - snd_seq_port_unlock(dport); - if (sender) - snd_seq_client_unlock(sender); - if (receiver) - snd_seq_client_unlock(receiver); +unlock_dport: + snd_seq_port_unlock(dport); +unlock_sport: + snd_seq_port_unlock(sport); +unlock_sender: + snd_seq_client_unlock(sender); +unlock_receiver: + snd_seq_client_unlock(receiver); +exit: return result; } -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 09:06:38 +0100 Four checks were repeated by the snd_seq_ioctl_subscribe_port() function despite of pointer determinations for the used variables at the beginning. * Adjust jump targets according to the Linux coding style convention. * Delete extra variable initialisations and the repeated checks which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/core/seq/seq_clientmgr.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index ba26d622cb71..b9518e7cb623 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1423,36 +1423,36 @@ static int snd_seq_ioctl_subscribe_port(struct snd_seq_client *client, { struct snd_seq_port_subscribe *subs = arg; int result = -EINVAL; - struct snd_seq_client *receiver = NULL, *sender = NULL; - struct snd_seq_client_port *sport = NULL, *dport = NULL; + struct snd_seq_client *receiver, *sender; + struct snd_seq_client_port *sport, *dport; if ((receiver = snd_seq_client_use_ptr(subs->dest.client)) == NULL) - goto __end; + goto exit; if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) - goto __end; + goto unlock_receiver; if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) - goto __end; + goto unlock_sender; if ((dport = snd_seq_port_use_ptr(receiver, subs->dest.port)) == NULL) - goto __end; + goto unlock_sport; result = check_subscription_permission(client, sport, dport, subs); if (result < 0) - goto __end; + goto unlock_dport; /* connect them */ result = snd_seq_port_connect(client, sender, sport, receiver, dport, subs); if (! result) /* broadcast announce */ snd_seq_client_notify_subscription(SNDRV_SEQ_ADDRESS_SUBSCRIBERS, 0, subs, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED); - __end: - if (sport) - snd_seq_port_unlock(sport); - if (dport) - snd_seq_port_unlock(dport); - if (sender) - snd_seq_client_unlock(sender); - if (receiver) - snd_seq_client_unlock(receiver); +unlock_dport: + snd_seq_port_unlock(dport); +unlock_sport: + snd_seq_port_unlock(sport); +unlock_sender: + snd_seq_client_unlock(sender); +unlock_receiver: + snd_seq_client_unlock(receiver); +exit: return result; } -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 09:44:22 +0100 The script "checkpatch.pl" pointed information out like the following. ERROR: do not use assignment in if condition Thus fix the affected source code places. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/core/seq/seq_clientmgr.c | 52 ++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index b9518e7cb623..519968841ccc 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -400,7 +400,11 @@ static ssize_t snd_seq_read(struct file *file, char __user *buf, size_t count, if (snd_BUG_ON(!client)) return -ENXIO; - if (!client->accept_input || (fifo = client->data.user.fifo) == NULL) + if (!client->accept_input) + return -ENXIO; + + fifo = client->data.user.fifo; + if (!fifo) return -ENXIO; if (atomic_read(&fifo->overflow) > 0) { @@ -419,9 +423,9 @@ static ssize_t snd_seq_read(struct file *file, char __user *buf, size_t count, int nonblock; nonblock = (file->f_flags & O_NONBLOCK) || result > 0; - if ((err = snd_seq_fifo_cell_out(fifo, &cell, nonblock)) < 0) { + err = snd_seq_fifo_cell_out(fifo, &cell, nonblock); + if (err < 0) break; - } if (snd_seq_ev_is_variable(&cell->event)) { struct snd_seq_event tmpev; tmpev = cell->event; @@ -948,7 +952,8 @@ static int snd_seq_client_enqueue_event(struct snd_seq_client *client, return err; /* we got a cell. enqueue it. */ - if ((err = snd_seq_enqueue_event(cell, atomic, hop)) < 0) { + err = snd_seq_enqueue_event(cell, atomic, hop); + if (err < 0) { snd_seq_cell_free(cell); return err; } @@ -1273,7 +1278,8 @@ static int snd_seq_ioctl_create_port(struct snd_seq_client *client, void *arg) return -EINVAL; } if (client->type == KERNEL_CLIENT) { - if ((callback = info->kernel) != NULL) { + callback = info->kernel; + if (callback) { if (callback->owner) port->owner = callback->owner; port->private_data = callback->private_data; @@ -1426,13 +1432,17 @@ static int snd_seq_ioctl_subscribe_port(struct snd_seq_client *client, struct snd_seq_client *receiver, *sender; struct snd_seq_client_port *sport, *dport; - if ((receiver = snd_seq_client_use_ptr(subs->dest.client)) == NULL) + receiver = snd_seq_client_use_ptr(subs->dest.client); + if (!receiver) goto exit; - if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) + sender = snd_seq_client_use_ptr(subs->sender.client); + if (!sender) goto unlock_receiver; - if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) + sport = snd_seq_port_use_ptr(sender, subs->sender.port); + if (!sport) goto unlock_sender; - if ((dport = snd_seq_port_use_ptr(receiver, subs->dest.port)) == NULL) + dport = snd_seq_port_use_ptr(receiver, subs->dest.port); + if (!dport) goto unlock_sport; result = check_subscription_permission(client, sport, dport, subs); @@ -1468,13 +1478,17 @@ static int snd_seq_ioctl_unsubscribe_port(struct snd_seq_client *client, struct snd_seq_client *receiver, *sender; struct snd_seq_client_port *sport, *dport; - if ((receiver = snd_seq_client_use_ptr(subs->dest.client)) == NULL) + receiver = snd_seq_client_use_ptr(subs->dest.client); + if (!receiver) goto exit; - if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) + sender = snd_seq_client_use_ptr(subs->sender.client); + if (!sender) goto unlock_receiver; - if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) + sport = snd_seq_port_use_ptr(sender, subs->sender.port); + if (!sport) goto unlock_sender; - if ((dport = snd_seq_port_use_ptr(receiver, subs->dest.port)) == NULL) + dport = snd_seq_port_use_ptr(receiver, subs->dest.port); + if (!dport) goto unlock_sport; result = check_subscription_permission(client, sport, dport, subs); @@ -1896,9 +1910,11 @@ static int snd_seq_ioctl_get_subscription(struct snd_seq_client *client, struct snd_seq_client_port *sport; struct snd_seq_subscribers *p; - if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) + sender = snd_seq_client_use_ptr(subs->sender.client); + if (!sender) goto exit; - if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) + sport = snd_seq_port_use_ptr(sender, subs->sender.port); + if (!sport) goto unlock_client; p = snd_seq_port_get_subscription(&sport->c_src, &subs->dest); if (p) { @@ -1928,9 +1944,11 @@ static int snd_seq_ioctl_query_subs(struct snd_seq_client *client, void *arg) struct list_head *p; int i; - if ((cptr = snd_seq_client_use_ptr(subs->root.client)) == NULL) + cptr = snd_seq_client_use_ptr(subs->root.client); + if (!cptr) goto exit; - if ((port = snd_seq_port_use_ptr(cptr, subs->root.port)) == NULL) + port = snd_seq_port_use_ptr(cptr, subs->root.port); + if (!port) goto unlock_client; switch (subs->type) { -- 2.11.0
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 10:05:29 +0100 A local variable was set to an error code in two cases before a concrete error situation was detected. Thus move the corresponding assignments into if branches to indicate a software failure there. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/oss/dmasound/dmasound_core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c index f4ee85a4c42f..2bdd1d619a7f 100644 --- a/sound/oss/dmasound/dmasound_core.c +++ b/sound/oss/dmasound/dmasound_core.c @@ -1269,13 +1269,15 @@ static int state_open(struct inode *inode, struct file *file) int ret; mutex_lock(&dmasound_core_mutex); - ret = -EBUSY; - if (state.busy) + if (state.busy) { + ret = -EBUSY; goto out; + } - ret = -ENODEV; - if (!try_module_get(dmasound.mach.owner)) + if (!try_module_get(dmasound.mach.owner)) { + ret = -ENODEV; goto out; + } state.ptr = 0; state.busy = 1; -- 2.11.0
SF Markus Elfring wrote:
A local variable was set to an error code in two cases before a concrete error situation was detected.
And why would that be a problem? http://yarchive.net/comp/linux/error_jumps.html
- ret = -EBUSY; - if (state.busy) + if (state.busy) { + ret = -EBUSY; goto out; + }
Regards, Clemens
From: Markus Elfring <elfring@users.sourceforge.net> Date: Tue, 24 Jan 2017 10:34:47 +0100 The script "checkpatch.pl" pointed information out like the following. ERROR: do not use assignment in if condition Thus fix affected source code places. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- sound/oss/dmasound/dmasound_core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c index 2bdd1d619a7f..a20bf3b94329 100644 --- a/sound/oss/dmasound/dmasound_core.c +++ b/sound/oss/dmasound/dmasound_core.c @@ -576,7 +576,9 @@ static ssize_t sq_write(struct file *file, const char __user *src, size_t uLeft, */ if (write_sq.locked == 0) { - if ((uWritten = sq_setup(&write_sq)) < 0) return uWritten ; + uWritten = sq_setup(&write_sq); + if (uWritten < 0) + return uWritten; uWritten = 0 ; } @@ -675,7 +677,8 @@ static unsigned int sq_poll(struct file *file, struct poll_table_struct *wait) int retVal; if (write_sq.locked == 0) { - if ((retVal = sq_setup(&write_sq)) < 0) + retVal = sq_setup(&write_sq); + if (retVal < 0) return retVal; return 0; } @@ -736,7 +739,8 @@ static int sq_open2(struct sound_queue *sq, struct file *file, fmode_t mode, can't be changed at the moment - but _could_ be perhaps in the setfragments ioctl. */ - if (( rc = sq_allocate_buffers(sq, numbufs, bufsize))) { + rc = sq_allocate_buffers(sq, numbufs, bufsize); + if (rc) { #if 0 /* blocking open() */ sq_wake_up(sq, file, mode); #else @@ -1407,12 +1411,14 @@ int dmasound_init(void) /* Set up sound queue, /dev/audio and /dev/dsp. */ /* Set default settings. */ - if ((res = sq_init()) < 0) - return res ; + res = sq_init(); + if (res < 0) + return res; /* Set up /dev/sndstat. */ - if ((res = state_init()) < 0) - return res ; + res = state_init(); + if (res < 0) + return res; /* Set up /dev/mixer. */ mixer_init(); @@ -1485,7 +1491,8 @@ static int dmasound_setup(char *str) numWriteBufs = ints[1]; /* fall through */ case 1: - if ((size = ints[2]) < 256) /* check for small buffer specs */ + size = ints[2]; + if (size < 256) /* check for small buffer specs */ size <<= 10 ; if (size < MIN_BUFSIZE || size > MAX_BUFSIZE) printk("dmasound_setup: invalid write buffer size, using default = %d\n", writeBufSize); -- 2.11.0
Date: Tue, 24 Jan 2017 11:02:34 +0100
Some update suggestions were taken into account from static source code analysis.
Markus Elfring (7): seq: Delete unnecessary checks in snd_seq_ioctl_query_subs() seq: Delete unnecessary checks in snd_seq_ioctl_get_subscription() seq: Delete unnecessary checks in snd_seq_ioctl_unsubscribe_port() seq: Delete unnecessary checks in snd_seq_ioctl_subscribe_port() seq: Adjust 16 function calls together with a variable assignment dmasound_core: Move two assignments for the variable "ret" in state_open() dmasound_core: Adjust six function calls together with a variable assignment
sound/core/seq/seq_clientmgr.c | 163 ++++++++++++++++++++----------------- sound/oss/dmasound/dmasound_core.c | 33 +++++--- 2 files changed, 109 insertions(+), 87 deletions(-)
Are these update suggestions worth for another look? https://lkml.org/lkml/2017/1/24/292 Regards, Markus
participants (2)
-
Clemens Ladisch -
SF Markus Elfring