| Message ID | 1428512838-2493-2-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Takashi > up_write(&card->controls_rwsem); > if (ret > 0) > snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); > -- > 2.1.0 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
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
At Fri, 10 Apr 2015 21:00:22 +0900, Takashi Sakamoto wrote: > > 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. If each of them should be applied individually, let me know. But I think this may have the very same problem as patch 1. > 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. No, the index number is just to allow multiple kctls with the same name. Using counts > 1 is only one of the implementations for multi indices. You can implement one kctl per index, too. (Many drivers do so.) Takashi > 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 >
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; up_write(&card->controls_rwsem); if (ret > 0) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
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(+)