Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

[Takashi Iwai]

At Wed, 09 Mar 2005 01:01:26 -0800,
Clemens Ladisch wrote:
> 
> Update of /cvsroot/alsa/alsa-kernel/core/oss
> In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv8486
> 
> Modified Files:
> 	mixer_oss.c 
> Log Message:
> Summary: fix forgotten release of semaphore in error path
> 
> In snd_mixer_oss_get_volume1_vol and snd_mixer_oss_put_volume1_vol,
> card->controls_rwsem wouldn't be released if the boolean type
> check fails.

IMO, it'd be better to use a plain if() rather than this macro.
It lowers readability.


Takashi

> 
> Index: mixer_oss.c
> ===================================================================
> RCS file: /cvsroot/alsa/alsa-kernel/core/oss/mixer_oss.c,v
> retrieving revision 1.38
> retrieving revision 1.39
> diff -u -r1.38 -r1.39
> --- mixer_oss.c	20 Jan 2005 17:14:44 -0000	1.38
> +++ mixer_oss.c	9 Mar 2005 09:01:23 -0000	1.39
> @@ -522,7 +522,7 @@
>  		goto __unalloc;
>  	snd_runtime_check(!kctl->info(kctl, uinfo), goto __unalloc);
>  	snd_runtime_check(!kctl->get(kctl, uctl), goto __unalloc);
> -	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, return);
> +	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, goto __unalloc);
>  	*left = snd_mixer_oss_conv1(uctl->value.integer.value[0], uinfo->value.integer.min, uinfo->value.integer.max, &pslot->volume[0]);
>  	if (uinfo->count > 1)
>  		*right = snd_mixer_oss_conv1(uctl->value.integer.value[1], uinfo->value.integer.min, uinfo->value.integer.max, &pslot->volume[1]);
> @@ -616,7 +616,7 @@
>  	if (uinfo == NULL || uctl == NULL)
>  		goto __unalloc;
>  	snd_runtime_check(!kctl->info(kctl, uinfo), goto __unalloc);
> -	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, return);
> +	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, goto __unalloc);
>  	uctl->value.integer.value[0] = snd_mixer_oss_conv2(left, uinfo->value.integer.min, uinfo->value.integer.max);
>  	if (uinfo->count > 1)
>  		uctl->value.integer.value[1] = snd_mixer_oss_conv2(right, uinfo->value.integer.min, uinfo->value.integer.max);
> 
> 
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Alsa-cvslog mailing list
> Alsa-cvslog@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/alsa-cvslog
> 


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

[Clemens Ladisch]

Takashi Iwai wrote:
> Clemens Ladisch wrote:
> > Summary: fix forgotten release of semaphore in error path
> >
> > In snd_mixer_oss_get_volume1_vol and snd_mixer_oss_put_volume1_vol,
> > card->controls_rwsem wouldn't be released if the boolean type
> > check fails.
> > ...
> > -	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, return);
> > +	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, goto __unalloc);
>
> IMO, it'd be better to use a plain if() rather than this macro.
> It lowers readability.

Yes, these lines are too long.

I made the minimal change to make it correct, just to be sure.  Feel
free to change it any way you want.  :-)


BTW: are there any plans to enforce the 80 columns limit of Linux
source code?  ;-)


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

[Takashi Iwai]

At Wed, 9 Mar 2005 10:20:58 +0100 (MET),
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Clemens Ladisch wrote:
> > > Summary: fix forgotten release of semaphore in error path
> > >
> > > In snd_mixer_oss_get_volume1_vol and snd_mixer_oss_put_volume1_vol,
> > > card->controls_rwsem wouldn't be released if the boolean type
> > > check fails.
> > > ...
> > > -	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, return);
> > > +	snd_runtime_check(uinfo->type != SNDRV_CTL_ELEM_TYPE_BOOLEAN || uinfo->value.integer.min != 0 || uinfo->value.integer.max != 1, goto __unalloc);
> >
> > IMO, it'd be better to use a plain if() rather than this macro.
> > It lowers readability.
> 
> Yes, these lines are too long.
> 
> I made the minimal change to make it correct, just to be sure.  Feel
> free to change it any way you want.  :-)

Hehe, I'm a broad-minded man, glad to give you a chance, too ;)


> BTW: are there any plans to enforce the 80 columns limit of Linux
> source code?  ;-)

Let's apply "indent -kr -i8 -l80" for all files :)

Well, the problem of snd_runtime_check() is not its length but that it
prevents to understand the code flow, just as you found in thie case.


Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

[Lee Revell]

On Wed, 2005-03-09 at 10:05 +0100, Takashi Iwai wrote:
> At Wed, 09 Mar 2005 01:01:26 -0800,
> Clemens Ladisch wrote:
> > 
> > Update of /cvsroot/alsa/alsa-kernel/core/oss
> > In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv8486
> > 
> > Modified Files:
> > 	mixer_oss.c 
> > Log Message:
> > Summary: fix forgotten release of semaphore in error path
> > 
> > In snd_mixer_oss_get_volume1_vol and snd_mixer_oss_put_volume1_vol,
> > card->controls_rwsem wouldn't be released if the boolean type
> > check fails.

Clemens, should this patch fix OSDL bug 4282?  You mentioned that was
related to a forgotten semaphore release in the same code.

Lee





-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: Re: [alsa-cvslog] CVS: alsa-kernel/core/oss mixer_oss.c,1.38,1.39

[Clemens Ladisch]

Lee Revell wrote:
> > Clemens Ladisch wrote:
> > > Summary: fix forgotten release of semaphore in error path
> > >
> > > In snd_mixer_oss_get_volume1_vol and snd_mixer_oss_put_volume1_vol,
> > > card->controls_rwsem wouldn't be released if the boolean type
> > > check fails.
>
> Clemens, should this patch fix OSDL bug 4282?  You mentioned that was
> related to a forgotten semaphore release in the same code.

Neither this fix nor my earlier one in snd_mixer_oss_build_input looks
like it could be related to that bug.  A semaphore that wasn't
released would result in a deadlock, not in a crash.

I would guess that the bug is caused by using something that hasn't
been initialized yet.


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click