[alsa-devel] [alsa-lib][PATCH] control: correct assertion in snd_ctl_elem_set_bytes()
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj); - if (size >= ARRAY_SIZE(obj->value.bytes.data)) { - assert(0); - return; - } + assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size); }
On Sun, 21 Feb 2016 17:54:09 +0100, Takashi Sakamoto wrote:
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
Admittedly this isn't an optimal code, but it has its purpose. The intention of the original code was to perform the check always but triggers the assert signal unless NDEBUG is set (remember that assert() is nop when NDEBUG is defined).
Takashi
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj);
- if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
assert(0);return;- }
- assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size);
}
-- 2.5.0
Hi,
On Feb 22 2016 02:46, Takashi Iwai wrote:
On Sun, 21 Feb 2016 17:54:09 +0100, Takashi Sakamoto wrote:
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
Admittedly this isn't an optimal code, but it has its purpose. The intention of the original code was to perform the check always but triggers the assert signal unless NDEBUG is set (remember that assert() is nop when NDEBUG is defined).
Although I realized the effect of NDEBUG for calls of asset(3), I didn't realize the intension to the code, because the other parts of 'src/control/control.c' are written without such intention. We can see many assert(3)s are just used without conditional statements.
Hm. I think it better to keep consistency of whole codes rather than such partial intensions. On the other hand, this patch changes library behaviour. It's not generally allowed, but here, I think it better because of the consistency and the actual effect.
Thanks
Takashi Sakamoto
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj);
- if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
assert(0);return;- }
- assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size);
}
-- 2.5.0
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 22 Feb 2016 01:34:30 +0100, Takashi Sakamoto wrote:
Hi,
On Feb 22 2016 02:46, Takashi Iwai wrote:
On Sun, 21 Feb 2016 17:54:09 +0100, Takashi Sakamoto wrote:
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
Admittedly this isn't an optimal code, but it has its purpose. The intention of the original code was to perform the check always but triggers the assert signal unless NDEBUG is set (remember that assert() is nop when NDEBUG is defined).
Although I realized the effect of NDEBUG for calls of asset(3), I didn't realize the intension to the code, because the other parts of 'src/control/control.c' are written without such intention. We can see many assert(3)s are just used without conditional statements.
Hm. I think it better to keep consistency of whole codes rather than such partial intensions. On the other hand, this patch changes library behaviour. It's not generally allowed, but here, I think it better because of the consistency and the actual effect.
Yeah, that's mostly OK to make it a simple assert.
Or, if I may judge only from my taste, even removing the assert would be nicer. Triggering assert() is one of the worst behavior as a base system library.
Takashi
Thanks
Takashi Sakamoto
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj);
- if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
assert(0);return;- }
- assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size);
}
-- 2.5.0
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Feb 22 2016 18:51, Takashi Iwai wrote:
On Mon, 22 Feb 2016 01:34:30 +0100, Takashi Sakamoto wrote:
Hi,
On Feb 22 2016 02:46, Takashi Iwai wrote:
On Sun, 21 Feb 2016 17:54:09 +0100, Takashi Sakamoto wrote:
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
Admittedly this isn't an optimal code, but it has its purpose. The intention of the original code was to perform the check always but triggers the assert signal unless NDEBUG is set (remember that assert() is nop when NDEBUG is defined).
Although I realized the effect of NDEBUG for calls of asset(3), I didn't realize the intension to the code, because the other parts of 'src/control/control.c' are written without such intention. We can see many assert(3)s are just used without conditional statements.
Hm. I think it better to keep consistency of whole codes rather than such partial intensions. On the other hand, this patch changes library behaviour. It's not generally allowed, but here, I think it better because of the consistency and the actual effect.
Yeah, that's mostly OK to make it a simple assert.
Or, if I may judge only from my taste, even removing the assert would be nicer. Triggering assert() is one of the worst behavior as a base system library.
I completely agree with the idea ;)
Although, it's out of my plan in this developing cycle. In this time, I hope just to apply this patch and go to my next work for control element set APIs. I should post revised version?
Regards
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj);
- if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
assert(0);return;- }
- assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size);
}
-- 2.5.0
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 22 Feb 2016 12:07:54 +0100, Takashi Sakamoto wrote:
On Feb 22 2016 18:51, Takashi Iwai wrote:
On Mon, 22 Feb 2016 01:34:30 +0100, Takashi Sakamoto wrote:
Hi,
On Feb 22 2016 02:46, Takashi Iwai wrote:
On Sun, 21 Feb 2016 17:54:09 +0100, Takashi Sakamoto wrote:
The assert(3) aborts running process when expression arguments is false (zero). Although, the snd_ctl_elem_set_bytes() has return statement after assert(3). It has meaningless.
Admittedly this isn't an optimal code, but it has its purpose. The intention of the original code was to perform the check always but triggers the assert signal unless NDEBUG is set (remember that assert() is nop when NDEBUG is defined).
Although I realized the effect of NDEBUG for calls of asset(3), I didn't realize the intension to the code, because the other parts of 'src/control/control.c' are written without such intention. We can see many assert(3)s are just used without conditional statements.
Hm. I think it better to keep consistency of whole codes rather than such partial intensions. On the other hand, this patch changes library behaviour. It's not generally allowed, but here, I think it better because of the consistency and the actual effect.
Yeah, that's mostly OK to make it a simple assert.
Or, if I may judge only from my taste, even removing the assert would be nicer. Triggering assert() is one of the worst behavior as a base system library.
I completely agree with the idea ;)
Although, it's out of my plan in this developing cycle. In this time, I hope just to apply this patch and go to my next work for control element set APIs. I should post revised version?
Well, removing assert() is somewhat radical and needs more consensus. So, although I'd love it, let's make this patch in. But, for that, I'd need a slightly corrected change log text. Care to resubmit after rephrasing?
thanks,
Takashi
Regards
This commit corrects the statements.
Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation') Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
src/control/control.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/control/control.c b/src/control/control.c index 328920d..70d168d 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size) { assert(obj);
- if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
assert(0);return;- }
- assert(size < ARRAY_SIZE(obj->value.bytes.data)); memcpy(obj->value.bytes.data, data, size);
}
-- 2.5.0
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (2)
-
Takashi Iwai -
Takashi Sakamoto