[PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Sakamoto]

When event originator doesn't set numerical ID in identical information,
the event data includes no numerical ID, thus userspace applications
cannot identify the control just by unique ID in event data.

This commit fix this bug so as the event data includes all of identical
information.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 6a72b3e..884fddd 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
 	}
 	ret = 1;
  unlock:
+	*id = kctl->id;
 	up_write(&card->controls_rwsem);
 	if (ret > 0)
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
-- 
2.1.0

Re: [PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Iwai]

At Tue, 10 Feb 2015 00:02:06 +0900,
Takashi Sakamoto wrote:
> 
> When event originator doesn't set numerical ID in identical information,
> the event data includes no numerical ID, thus userspace applications
> cannot identify the control just by unique ID in event data.
> 
> This commit fix this bug so as the event data includes all of identical
> information.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

You can't overwrite id pointer in this case.  It's a caller's object.

One way to fix would be to copy the id instance.  Another way would be
to change up/down_write() with _read(), and include snd_ctl_notify()
call with kctl->id inside the semaphore lock.

The former would be less changes but consume the stack significantly.


thanks,

Takashi


> ---
>  sound/core/control.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 6a72b3e..884fddd 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>  	}
>  	ret = 1;
>   unlock:
> +	*id = kctl->id;
>  	up_write(&card->controls_rwsem);
>  	if (ret > 0)
>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
> -- 
> 2.1.0
> 

Re: [PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Sakamoto]

On Feb 10 2015 00:28, Takashi Iwai wrote:
> At Tue, 10 Feb 2015 00:02:06 +0900,
> Takashi Sakamoto wrote:
>>
>> When event originator doesn't set numerical ID in identical information,
>> the event data includes no numerical ID, thus userspace applications
>> cannot identify the control just by unique ID in event data.
>>
>> This commit fix this bug so as the event data includes all of identical
>> information.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>
> You can't overwrite id pointer in this case.  It's a caller's object.

Exactly. I missed it...

> One way to fix would be to copy the id instance.  Another way would be
> to change up/down_write() with _read(), and include snd_ctl_notify()
> call with kctl->id inside the semaphore lock.
>
> The former would be less changes but consume the stack significantly.

If any sub-effects are allowed, we can use caller's data via the 
pointer, like:
memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id);
Then:
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);

But this is not better. Mmm...

> thanks,
>
> Takashi
>
>
>> ---
>>   sound/core/control.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index 6a72b3e..884fddd 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>>   	}
>>   	ret = 1;
>>    unlock:
>> +	*id = kctl->id;
>>   	up_write(&card->controls_rwsem);
>>   	if (ret > 0)
>>   		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
>> --
>> 2.1.0

Re: [PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Iwai]

At Tue, 10 Feb 2015 01:01:15 +0900,
Takashi Sakamoto wrote:
> 
> On Feb 10 2015 00:28, Takashi Iwai wrote:
> > At Tue, 10 Feb 2015 00:02:06 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> When event originator doesn't set numerical ID in identical information,
> >> the event data includes no numerical ID, thus userspace applications
> >> cannot identify the control just by unique ID in event data.
> >>
> >> This commit fix this bug so as the event data includes all of identical
> >> information.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >
> > You can't overwrite id pointer in this case.  It's a caller's object.
> 
> Exactly. I missed it...
> 
> > One way to fix would be to copy the id instance.  Another way would be
> > to change up/down_write() with _read(), and include snd_ctl_notify()
> > call with kctl->id inside the semaphore lock.
> >
> > The former would be less changes but consume the stack significantly.
> 
> If any sub-effects are allowed, we can use caller's data via the 
> pointer, like:
> memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id);
> Then:
> snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);

That's what I suggested as the first example.


Takashi

Re: [PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Iwai]

At Mon, 09 Feb 2015 17:02:51 +0100,
Takashi Iwai wrote:
> 
> At Tue, 10 Feb 2015 01:01:15 +0900,
> Takashi Sakamoto wrote:
> > 
> > On Feb 10 2015 00:28, Takashi Iwai wrote:
> > > At Tue, 10 Feb 2015 00:02:06 +0900,
> > > Takashi Sakamoto wrote:
> > >>
> > >> When event originator doesn't set numerical ID in identical information,
> > >> the event data includes no numerical ID, thus userspace applications
> > >> cannot identify the control just by unique ID in event data.
> > >>
> > >> This commit fix this bug so as the event data includes all of identical
> > >> information.
> > >>
> > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > >
> > > You can't overwrite id pointer in this case.  It's a caller's object.
> > 
> > Exactly. I missed it...
> > 
> > > One way to fix would be to copy the id instance.  Another way would be
> > > to change up/down_write() with _read(), and include snd_ctl_notify()
> > > call with kctl->id inside the semaphore lock.
> > >
> > > The former would be less changes but consume the stack significantly.
> > 
> > If any sub-effects are allowed, we can use caller's data via the 
> > pointer, like:
> > memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id);
> > Then:
> > snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
> 
> That's what I suggested as the first example.

... and I think this is a better way in the end.  Simpler, better.
The stack usage is very likely acceptable.


Takashi

Re: [PATCH] ALSA: control: confirm to return all identical information in 'activate' event data

[Takashi Sakamoto]

On Feb 10 2015 01:18, Takashi Iwai wrote:
> At Mon, 09 Feb 2015 17:02:51 +0100,
> Takashi Iwai wrote:
>>
>> At Tue, 10 Feb 2015 01:01:15 +0900,
>> Takashi Sakamoto wrote:
>>>
>>> On Feb 10 2015 00:28, Takashi Iwai wrote:
>>>> At Tue, 10 Feb 2015 00:02:06 +0900,
>>>> Takashi Sakamoto wrote:
>>>>>
>>>>> When event originator doesn't set numerical ID in identical information,
>>>>> the event data includes no numerical ID, thus userspace applications
>>>>> cannot identify the control just by unique ID in event data.
>>>>>
>>>>> This commit fix this bug so as the event data includes all of identical
>>>>> information.
>>>>>
>>>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>>>
>>>> You can't overwrite id pointer in this case.  It's a caller's object.
>>>
>>> Exactly. I missed it...
>>>
>>>> One way to fix would be to copy the id instance.  Another way would be
>>>> to change up/down_write() with _read(), and include snd_ctl_notify()
>>>> call with kctl->id inside the semaphore lock.
>>>>
>>>> The former would be less changes but consume the stack significantly.
>>>
>>> If any sub-effects are allowed, we can use caller's data via the
>>> pointer, like:
>>> memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id);
>>> Then:
>>> snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
>>
>> That's what I suggested as the first example.

I misunderstood to use heap.

> ... and I think this is a better way in the end.  Simpler, better.
> The stack usage is very likely acceptable.

I think so. I'll submit again with some comments about the sub-effect to 
callers.


Thanks

Takashi Sakamoto