[PATCH 0/5 alsa-lib] amixer fixes for enumerated elements

[PATCH 0/5 alsa-lib] amixer fixes for enumerated elements

[Takashi Sakamoto]

The amixer still has issues according to sset operation to enumerated
items. The issues are:
 - Fail to handle omitted value of each channel.
 - The name of enumerated element is truncated.
 - Unclear separators for values of channels.

This patchset fixes these issues, including a small code
refactoring for better reading.

Takashi Sakamoto (5):
  amixer: gather local variables in the beginning of functions
  amixer: fix a bug to handle omitted value in parameter for enumerated
    element
  amixer: expand local storage for item name according to kernel code
  amixer: arrange validation logic
  amixer: use the same characters for separator.

 amixer/amixer.c | 60 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH 1/5] amixer: gather local variables in the beginning of functions

[Takashi Sakamoto]

Following to basic principles of C language programing.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amixer/amixer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/amixer/amixer.c b/amixer/amixer.c
index 36c92eb..ff1a927 100644
--- a/amixer/amixer.c
+++ b/amixer/amixer.c
@@ -1278,13 +1278,16 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 
 static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
 {
-	unsigned int idx, item = 0;
+	unsigned int idx;
+	unsigned int item = 0;
 	int check_flag = ignore_error ? 0 : -1;
+	int ival;
+	char *ptr;
 
 	for (idx = 1; idx < argc; idx++) {
-		char *ptr = argv[idx];
+		ptr = argv[idx];
 		while (*ptr) {
-			int ival = get_enum_item_index(elem, &ptr);
+			ival = get_enum_item_index(elem, &ptr);
 			if (ival < 0)
 				return check_flag;
 			if (snd_mixer_selem_set_enum_item(elem, item++, ival) >= 0)
-- 
2.1.0

[PATCH 2/5] amixer: fix a bug to handle omitted value in parameter for enumerated element

[Takashi Sakamoto]

In processing of 'sset' operation for enumerated elements, while loop is
used to skip separators for arguments which shows the value of each channel.
But the actual channel is not incremented, thus this sample causes wrong
result that D is set to third channel, instead of the last channel.

$ amixer sset enum-element-13,1019 A,B,,D

Furthermore, when the value of first channel is omitted, the command causes
'Invalid command' output.

This commit fixes these bug, by incrementing channel number according to
the loop and move the loop to the beginning of outer loop.

Fixes: 1a19ec153850('amixer: Don't set only the first item in sset_enum()')
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amixer/amixer.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/amixer/amixer.c b/amixer/amixer.c
index ff1a927..a3de375 100644
--- a/amixer/amixer.c
+++ b/amixer/amixer.c
@@ -1279,22 +1279,28 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
 {
 	unsigned int idx;
-	unsigned int item = 0;
+	unsigned int chn;
 	int check_flag = ignore_error ? 0 : -1;
 	int ival;
 	char *ptr;
 
 	for (idx = 1; idx < argc; idx++) {
 		ptr = argv[idx];
+		chn = 0;
 		while (*ptr) {
+			/* Skip separators between the value of each channel. */
+			while (*ptr == ',' || isspace(*ptr)) {
+				ptr++;
+				chn++;
+			}
+
+			/* Get index for given argument as enumerated item. */
 			ival = get_enum_item_index(elem, &ptr);
 			if (ival < 0)
 				return check_flag;
-			if (snd_mixer_selem_set_enum_item(elem, item++, ival) >= 0)
+
+			if (snd_mixer_selem_set_enum_item(elem, chn, ival) >= 0)
 				check_flag = 1;
-			/* skip separators */
-			while (*ptr == ',' || isspace(*ptr))
-				ptr++;
 		}
 	}
 	return check_flag;
-- 
2.1.0

[PATCH 3/5] amixer: expand local storage for item name according to kernel code

[Takashi Sakamoto]

According to kernel code (snd_ctl_elem_init_enum_names() in
sound/core/control.c), the maximum length of item name is 63 characters
(+ 1 terminator = 64 bytes). But current amixer implementation
uses 40 bytes. This causes name truncation and fail to operation.

This commit fixes this bug by expanding the length of local variables.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amixer/amixer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/amixer/amixer.c b/amixer/amixer.c
index a3de375..65ebf20 100644
--- a/amixer/amixer.c
+++ b/amixer/amixer.c
@@ -812,7 +812,11 @@ static int show_selem(snd_mixer_t *handle, snd_mixer_selem_id_t *id, const char
 		if (snd_mixer_selem_is_enumerated(elem)) {
 			int i, items;
 			unsigned int idx;
-			char itemname[40];
+			/*
+			 * See snd_ctl_elem_init_enum_names() in
+			 * sound/core/control.c.
+			 */
+			char itemname[64];
 			items = snd_mixer_selem_get_enum_items(elem);
 			printf("  Items:");
 			for (i = 0; i < items; i++) {
@@ -1255,7 +1259,9 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 {
 	char *ptr = *ptrp;
 	int items, i, len;
-	char name[40];
+
+	/* See snd_ctl_elem_init_enum_names() in sound/core/control.c. */
+	char name[64];
 	
 	items = snd_mixer_selem_get_enum_items(elem);
 	if (items <= 0)
@@ -1264,6 +1270,7 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 	for (i = 0; i < items; i++) {
 		if (snd_mixer_selem_get_enum_item_name(elem, i, sizeof(name)-1, name) < 0)
 			continue;
+
 		len = strlen(name);
 		if (! strncmp(name, ptr, len)) {
 			if (! ptr[len] || ptr[len] == ',' || ptr[len] == '\n') {
-- 
2.1.0

[PATCH 4/5] amixer: arrange validation logic

[Takashi Sakamoto]

For better reading and easy understanding.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amixer/amixer.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/amixer/amixer.c b/amixer/amixer.c
index 65ebf20..aec8d01 100644
--- a/amixer/amixer.c
+++ b/amixer/amixer.c
@@ -1271,14 +1271,18 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 		if (snd_mixer_selem_get_enum_item_name(elem, i, sizeof(name)-1, name) < 0)
 			continue;
 
+		/* Check given strings itself. */
 		len = strlen(name);
-		if (! strncmp(name, ptr, len)) {
-			if (! ptr[len] || ptr[len] == ',' || ptr[len] == '\n') {
-				ptr += len;
-				*ptrp = ptr;
-				return i;
-			}
-		}
+		if (strncmp(name, ptr, len) != 0)
+			continue;
+
+		/* Lack of separators between channels. */
+		if (ptr[len] != '\0' && ptr[len] != ',' && ptr[len] != '\n')
+			continue;
+
+		/* OK. The string is exactly one of items. */
+		*ptrp = ptr + len;
+		return i;
 	}
 	return -1;
 }
@@ -1294,9 +1298,9 @@ static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
 	for (idx = 1; idx < argc; idx++) {
 		ptr = argv[idx];
 		chn = 0;
-		while (*ptr) {
+		while (ptr[0] != '\0') {
 			/* Skip separators between the value of each channel. */
-			while (*ptr == ',' || isspace(*ptr)) {
+			while (ptr[0] == ',' || isspace(ptr[0])) {
 				ptr++;
 				chn++;
 			}
@@ -1304,8 +1308,12 @@ static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
 			/* Get index for given argument as enumerated item. */
 			ival = get_enum_item_index(elem, &ptr);
 			if (ival < 0)
-				return check_flag;
+				break;
 
+			/*
+			 * Stand success flag when at least one channel is
+			 * changed.
+			 */
 			if (snd_mixer_selem_set_enum_item(elem, chn, ival) >= 0)
 				check_flag = 1;
 		}
-- 
2.1.0

[PATCH 5/5] amixer: use the same characters for separator.

[Takashi Sakamoto]

The arguments are parsed as the value of each channel for enumerated
element in sset_enum() and get_enum_item_index(), the former is
a caller and the latter is a callee. Both of them evaluate the string
but use different characters for separator. This brings just cofusion
to users.

This commit fix this bug, by changing callee's characters according to
caller's characters.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amixer/amixer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/amixer/amixer.c b/amixer/amixer.c
index aec8d01..fa4bde1 100644
--- a/amixer/amixer.c
+++ b/amixer/amixer.c
@@ -1276,8 +1276,8 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
 		if (strncmp(name, ptr, len) != 0)
 			continue;
 
-		/* Lack of separators between channels. */
-		if (ptr[len] != '\0' && ptr[len] != ',' && ptr[len] != '\n')
+		/* Lack of string terminator and separator between channels. */
+		if (ptr[len] != '\0' && ptr[len] != ',' && !isspace(ptr[len]))
 			continue;
 
 		/* OK. The string is exactly one of items. */
-- 
2.1.0

Re: [PATCH 0/5 alsa-utils] amixer fixes for enumerated elements

[Takashi Sakamoto]

> [PATCH 0/5 alsa-lib] amixer fixes for enumerated elements

This is for 'alsa-utils', again...
(I seems to be obsessed by it...)

On Apr 09 2015 01:30, Takashi Sakamoto wrote:
> The amixer still has issues according to sset operation to enumerated
> items. The issues are:
>  - Fail to handle omitted value of each channel.
>  - The name of enumerated element is truncated.
>  - Unclear separators for values of channels.
> 
> This patchset fixes these issues, including a small code
> refactoring for better reading.
> 
> Takashi Sakamoto (5):
>   amixer: gather local variables in the beginning of functions
>   amixer: fix a bug to handle omitted value in parameter for enumerated
>     element
>   amixer: expand local storage for item name according to kernel code
>   amixer: arrange validation logic
>   amixer: use the same characters for separator.
> 
>  amixer/amixer.c | 60 ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 18 deletions(-)

Re: [PATCH 1/5] amixer: gather local variables in the beginning of functions

[Takashi Iwai]

At Thu,  9 Apr 2015 01:30:54 +0900,
Takashi Sakamoto wrote:
> 
> Following to basic principles of C language programing.

It's pretty normal (and often more useful) to define a variable in a
block.


Takashi

> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  amixer/amixer.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/amixer/amixer.c b/amixer/amixer.c
> index 36c92eb..ff1a927 100644
> --- a/amixer/amixer.c
> +++ b/amixer/amixer.c
> @@ -1278,13 +1278,16 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
>  
>  static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
>  {
> -	unsigned int idx, item = 0;
> +	unsigned int idx;
> +	unsigned int item = 0;
>  	int check_flag = ignore_error ? 0 : -1;
> +	int ival;
> +	char *ptr;
>  
>  	for (idx = 1; idx < argc; idx++) {
> -		char *ptr = argv[idx];
> +		ptr = argv[idx];
>  		while (*ptr) {
> -			int ival = get_enum_item_index(elem, &ptr);
> +			ival = get_enum_item_index(elem, &ptr);
>  			if (ival < 0)
>  				return check_flag;
>  			if (snd_mixer_selem_set_enum_item(elem, item++, ival) >= 0)
> -- 
> 2.1.0
> 

Re: [PATCH 2/5] amixer: fix a bug to handle omitted value in parameter for enumerated element

[Takashi Iwai]

At Thu,  9 Apr 2015 01:30:55 +0900,
Takashi Sakamoto wrote:
> 
> In processing of 'sset' operation for enumerated elements, while loop is
> used to skip separators for arguments which shows the value of each channel.
> But the actual channel is not incremented, thus this sample causes wrong
> result that D is set to third channel, instead of the last channel.
> 
> $ amixer sset enum-element-13,1019 A,B,,D
> 
> Furthermore, when the value of first channel is omitted, the command causes
> 'Invalid command' output.
> 
> This commit fixes these bug, by incrementing channel number according to
> the loop and move the loop to the beginning of outer loop.
> 
> Fixes: 1a19ec153850('amixer: Don't set only the first item in sset_enum()')
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  amixer/amixer.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/amixer/amixer.c b/amixer/amixer.c
> index ff1a927..a3de375 100644
> --- a/amixer/amixer.c
> +++ b/amixer/amixer.c
> @@ -1279,22 +1279,28 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
>  static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
>  {
>  	unsigned int idx;
> -	unsigned int item = 0;
> +	unsigned int chn;
>  	int check_flag = ignore_error ? 0 : -1;
>  	int ival;
>  	char *ptr;
>  
>  	for (idx = 1; idx < argc; idx++) {
>  		ptr = argv[idx];
> +		chn = 0;
>  		while (*ptr) {
> +			/* Skip separators between the value of each channel. */
> +			while (*ptr == ',' || isspace(*ptr)) {
> +				ptr++;
> +				chn++;

This should be handled only for commas, not for spaces.  Two spaces
don't mean to skip two channels, usually.  Also, the space at the
beginning should be just discarded.  The trailing spaces should be
seen as a separator.


Takashi

> +			}
> +
> +			/* Get index for given argument as enumerated item. */
>  			ival = get_enum_item_index(elem, &ptr);
>  			if (ival < 0)
>  				return check_flag;
> -			if (snd_mixer_selem_set_enum_item(elem, item++, ival) >= 0)
> +
> +			if (snd_mixer_selem_set_enum_item(elem, chn, ival) >= 0)
>  				check_flag = 1;
> -			/* skip separators */
> -			while (*ptr == ',' || isspace(*ptr))
> -				ptr++;
>  		}
>  	}
>  	return check_flag;
> -- 
> 2.1.0
> 

Re: [PATCH 4/5] amixer: arrange validation logic

[Takashi Iwai]

At Thu,  9 Apr 2015 01:30:57 +0900,
Takashi Sakamoto wrote:
> 
> For better reading and easy understanding.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  amixer/amixer.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/amixer/amixer.c b/amixer/amixer.c
> index 65ebf20..aec8d01 100644
> --- a/amixer/amixer.c
> +++ b/amixer/amixer.c
> @@ -1271,14 +1271,18 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
>  		if (snd_mixer_selem_get_enum_item_name(elem, i, sizeof(name)-1, name) < 0)
>  			continue;
>  
> +		/* Check given strings itself. */
>  		len = strlen(name);
> -		if (! strncmp(name, ptr, len)) {
> -			if (! ptr[len] || ptr[len] == ',' || ptr[len] == '\n') {
> -				ptr += len;
> -				*ptrp = ptr;
> -				return i;
> -			}
> -		}
> +		if (strncmp(name, ptr, len) != 0)
> +			continue;
> +
> +		/* Lack of separators between channels. */
> +		if (ptr[len] != '\0' && ptr[len] != ',' && ptr[len] != '\n')
> +			continue;

In general, the negation makes the logic harder to follow.
That is, a code like below (it's almost same as the original one) is
easier, more understandable.

		/* EOS or separator? */
		if (ptr[len] || ptr[len] == ',' || ptr[len] == '\n') {
			*ptrp = ptr + len;
			return i;
		}


>  	}
>  	return -1;
>  }
> @@ -1294,9 +1298,9 @@ static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
>  	for (idx = 1; idx < argc; idx++) {
>  		ptr = argv[idx];
>  		chn = 0;
> -		while (*ptr) {
> +		while (ptr[0] != '\0') {

ptr[0] is less common expression than *ptr, IMO.


Takashi

>  			/* Skip separators between the value of each channel. */
> -			while (*ptr == ',' || isspace(*ptr)) {
> +			while (ptr[0] == ',' || isspace(ptr[0])) {
>  				ptr++;
>  				chn++;
>  			}
> @@ -1304,8 +1308,12 @@ static int sset_enum(snd_mixer_elem_t *elem, unsigned int argc, char **argv)
>  			/* Get index for given argument as enumerated item. */
>  			ival = get_enum_item_index(elem, &ptr);
>  			if (ival < 0)
> -				return check_flag;
> +				break;
>  
> +			/*
> +			 * Stand success flag when at least one channel is
> +			 * changed.
> +			 */
>  			if (snd_mixer_selem_set_enum_item(elem, chn, ival) >= 0)
>  				check_flag = 1;
>  		}
> -- 
> 2.1.0
> 

Re: [PATCH 3/5] amixer: expand local storage for item name according to kernel code

[Takashi Iwai]

At Thu,  9 Apr 2015 01:30:56 +0900,
Takashi Sakamoto wrote:
> 
> According to kernel code (snd_ctl_elem_init_enum_names() in
> sound/core/control.c), the maximum length of item name is 63 characters
> (+ 1 terminator = 64 bytes). But current amixer implementation
> uses 40 bytes. This causes name truncation and fail to operation.
> 
> This commit fixes this bug by expanding the length of local variables.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  amixer/amixer.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/amixer/amixer.c b/amixer/amixer.c
> index a3de375..65ebf20 100644
> --- a/amixer/amixer.c
> +++ b/amixer/amixer.c
> @@ -812,7 +812,11 @@ static int show_selem(snd_mixer_t *handle, snd_mixer_selem_id_t *id, const char
>  		if (snd_mixer_selem_is_enumerated(elem)) {
>  			int i, items;
>  			unsigned int idx;
> -			char itemname[40];
> +			/*
> +			 * See snd_ctl_elem_init_enum_names() in
> +			 * sound/core/control.c.
> +			 */
> +			char itemname[64];
>  			items = snd_mixer_selem_get_enum_items(elem);
>  			printf("  Items:");
>  			for (i = 0; i < items; i++) {
> @@ -1255,7 +1259,9 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
>  {
>  	char *ptr = *ptrp;
>  	int items, i, len;
> -	char name[40];
> +
> +	/* See snd_ctl_elem_init_enum_names() in sound/core/control.c. */
> +	char name[64];
>  	
>  	items = snd_mixer_selem_get_enum_items(elem);
>  	if (items <= 0)
> @@ -1264,6 +1270,7 @@ static int get_enum_item_index(snd_mixer_elem_t *elem, char **ptrp)
>  	for (i = 0; i < items; i++) {
>  		if (snd_mixer_selem_get_enum_item_name(elem, i, sizeof(name)-1, name) < 0)
>  			continue;
> +
>  		len = strlen(name);
>  		if (! strncmp(name, ptr, len)) {
>  			if (! ptr[len] || ptr[len] == ',' || ptr[len] == '\n') {
> -- 
> 2.1.0
>