[alsa-devel] [alsa-lib][PATCH] control: correct assertion in snd_ctl_elem_set_bytes()

[Takashi Iwai]

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 at 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 at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 
>