Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical information

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, clemens@ladisch.de
Subject: Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical	information
Date: Fri, 10 Apr 2015 21:00:22 +0900	[thread overview]
Message-ID: <5527BB56.1090407@sakamocchi.jp> (raw)
In-Reply-To: <s5h61957thw.wl-tiwai@suse.de>

On 2015年04月09日 14:36, Takashi Iwai wrote:
> At Thu,  9 Apr 2015 02:07:15 +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>
>> ---
>>  sound/core/control.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index d677c27..6d12e85 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -578,6 +578,7 @@ error:
>>   *
>>   * Finds the control instance with the given id, and activate or
>>   * inactivate the control together with notification, if changed.
>> + * The given ID data is filled by full information.
>>   *
>>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
>>   */
>> @@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
>>  	}
>>  	ret = 1;
>>   unlock:
>> +	*id = kctl->id;
> 
> This isn't always correct.  When the element is looked by a numid and
> a kctl has multiple items (count > 0), this will overwrite with a
> wrong numid.

It also overwrites with a wrong index, too. I should have used a
combination of snd_ctl_get_ioff() and snd_ctl_build_ioff() for this
purpose. Thanks.

> Takashi
> 
>>  	up_write(&card->controls_rwsem);
>>  	if (ret > 0)
>>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
>> -- 
>> 2.1.0

Well, I'll re-post the patch 01 and 04 with these fixes because the
others has somewhat changes of API behaviour, expecially patch 03. I'll
postpone it to next developing period.

But how about patch 02? I think filling all identical information gives
advantage to userspace applications, instead of a part of given
information given.

And I have no confidence about giving index value in add operations for
userspace. In my understanding, the index is an offset from the first
element in the element set. But actually, via the add operation, we can
add new userspace element with an arbitrary index, like:

$ amixer controls
(added by an operation with index=0. As a result, 11 is given as numid
for this element set.)
numid=11,iface=MIXER,name='my-enum-element'
numid=12,iface=MIXER,name='my-enum-element',index=1
numid=13,iface=MIXER,name='my-enum-element',index=2
numid=14,iface=MIXER,name='my-enum-element',index=3
 - there're some other elements. -
(added by another operation with index=5. As a result, 21 is given as
numid for this element set.)
numid=21,iface=MIXER,name='my-enum-element',index=4
numid=22,iface=MIXER,name='my-enum-element',index=5
numid=23,iface=MIXER,name='my-enum-element',index=6
numid=24,iface=MIXER,name='my-enum-element',index=7
(FYI, addition with index=1-8 causes EBUSY.)

Of cource, we can do this.
(added by an operation with 100. As a result, 0 is given as numid for
this element set.)
numid=0,iface=MIXER,name='my-enum-element',index=100
numid=1,iface=MIXER,name='my-enum-element',index=101
numid=2,iface=MIXER,name='my-enum-element',index=102
numid=3,iface=MIXER,name='my-enum-element',index=103

Is this behaviour legal or not?

# I'm sorry to post these questions just before opening next merge
window but it takes me
# a long time to understand ALSA control interface and prepare for some
helper programs
# to confirm its behaviours...

Thanks

Takashi Sakamoto

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel