| Message ID | 20210318103013.265264-1-o-takashi@sakamocchi.jp |
|---|---|
| Headers | show |
| Series | add API of equality and comparison for a pair of control element IDs | expand |
Dne 18. 03. 21 v 11:30 Takashi Sakamoto napsal(a): > Hi, > > This patchset is a fix for bug issued in the message thread[1]. > > In this development period, alsa-lib got new API as implementation for > one of comparison algorithms to a pair of control element IDs. However, > it has several issues. > > At first, the name, 'snd_ctl_elem_id_compare()', is inappropriate since it > implements one of comparison algorithms. The name itself implies the > algorithm is single and unique for control element ID. However, the > target structure, 'struct snd_ctl_elem_id', is hybrid and compound one. > We can not find such single and unique comparison algorithm for it. > > Secondary, it subtracts a pair of values in fields of 'unsigned int' type > in storage size of the type. It brings integer overflow. I don't think that this extra handling is really required. The unsigned / signed conversions are well known and the overflow results with a negative signed value. Why add more branches to the instruction chain? > Tertiary, it has simple bug to compare subdevice field in the same structure. Good catch. > Essentially, equality is different from comparison. In a point of programming, Yes, but in this case, there's no benefit to have things separated. Add inline functions if you like to create application helpers which may be replaced by the functions in the future. If we really need a super CPU optimized equality check functions, we can add them later. The full compare functions must return zero, if the values are equal. I prefer minimal API changes here. > implementation for comparison algorithm can have more overhead than > implementation for equality. In this meaning, it's better to add different API > for them. > > This patchset adds new API below: > > * for equality > * snd_ctl_elem_id_equal_by_numid() > * snd_ctl_elem_id_equal_by_tuple() > * for each comparison algorithm > * snd_ctl_elem_id_compare_by_numid() > * snd_ctl_elem_id_compare_by_tuple_arithmetic() > > I've got bothered to decide the name of API for the case to use tuples. > Here I use the word, 'tuple', which comes from documentation of alsa-lib[2]. The tuple naming is a bit new and I would prefer fields or so here. The ID is represented by the number or group of fields. Also arithmetic suffix makes the function name longer. The new function may use other name (if any will be implemented later). Also, I don't like l and r argument naming. We should follow strcmp() and don't try to invent something new here. Also my old function should be just renamed. No add + remove is necessary for the modifications of the touched code. Resume: 1) rename snd_ctl_elem_id_compare() to snd_ctl_elem_id_compare_fields() 2) add snd_ctl_elem_id_compare_by_numid() .. optionally ... 3) add snd_ctl_elem_id_equal_by_numid() - as inline using compare fcn 4) add snd_ctl_elem_id_equal_by_fields() - also inline Jaroslav
On Fri, Mar 19, 2021 at 01:37:15AM +0900, Takashi Sakamoto wrote: > As I described, your old implementation is not acceptable just by renaming. > Although the idea of inline definitions is itself preferable. I suspect whether > inline definition for your comparison algorithm is really less overhead than > function call. > > Anyway I'll post revised version of patchset later. Oops. These APIs have arguments with opaque pointers. In the case, inline definition is not available since the layout of structure underlying the pointer is not available for userspace applications. Thus the rest of issue is whether to use 'tuple' or 'fields' in the name of new API. In my opinion, 'fields' is generic expression and could give impression to application developers that it includes numid field as well. On the other hand, 'tuple' is weak expression somehow and the developers easily find its meaning in alsa-lib documentation (see line 80). When considering about helpfulness to developers, 'tuple' seems to be better than 'fields'. Thanks Takashi Sakamoto
Dne 18. 03. 21 v 17:56 Takashi Sakamoto napsal(a): > On Fri, Mar 19, 2021 at 01:37:15AM +0900, Takashi Sakamoto wrote: >> As I described, your old implementation is not acceptable just by renaming. >> Although the idea of inline definitions is itself preferable. I suspect whether >> inline definition for your comparison algorithm is really less overhead than >> function call. >> >> Anyway I'll post revised version of patchset later. > > Oops. These APIs have arguments with opaque pointers. In the case, > inline definition is not available since the layout of structure underlying > the pointer is not available for userspace applications. Thus the rest of > issue is whether to use 'tuple' or 'fields' in the name of new API. > > In my opinion, 'fields' is generic expression and could give impression to > application developers that it includes numid field as well. On the other > hand, 'tuple' is weak expression somehow and the developers easily find > its meaning in alsa-lib documentation (see line 80). When considering about > helpfulness to developers, 'tuple' seems to be better than 'fields'. With this logic, we can just create snd_ctl_id_compare1, snd_ctl_id_compare2 functions to force developers to go to the docs. Perhaps, snd_ctl_id_compare_full() may be better. Tuple is a generic set of fields, so there's no change. Again, I don't expect to add other comparison functions soon. Jaroslav
Hi Jaroslav, Sorry to be my late reply but I have some private troubles (to search my domestic cat back to the street several days ago.). On Thu, Mar 18, 2021 at 06:04:36PM +0100, Jaroslav Kysela wrote: > Dne 18. 03. 21 v 17:37 Takashi Sakamoto napsal(a): > > On Thu, Mar 18, 2021 at 12:42:30PM +0100, Jaroslav Kysela wrote: > >> Dne 18. 03. 21 v 11:30 Takashi Sakamoto napsal(a): > >>> Hi, > >>> > >>> This patchset is a fix for bug issued in the message thread[1]. > >>> > >>> In this development period, alsa-lib got new API as implementation for > >>> one of comparison algorithms to a pair of control element IDs. However, > >>> it has several issues. > >>> > >>> At first, the name, 'snd_ctl_elem_id_compare()', is inappropriate since it > >>> implements one of comparison algorithms. The name itself implies the > >>> algorithm is single and unique for control element ID. However, the > >>> target structure, 'struct snd_ctl_elem_id', is hybrid and compound one. > >>> We can not find such single and unique comparison algorithm for it. > >>> > >>> Secondary, it subtracts a pair of values in fields of 'unsigned int' type > >>> in storage size of the type. It brings integer overflow. > >> > >> I don't think that this extra handling is really required. The unsigned / > >> signed conversions are well known and the overflow results with a negative > >> signed value. Why add more branches to the instruction chain? > > > > For this kind of question, it's preferable to write actual test: > > > > ``` > > int main() > > { > > snd_ctl_elem_id_t *l, *r; > > > > snd_ctl_elem_id_alloca(&l); > > snd_ctl_elem_id_alloca(&r); > > > > snd_ctl_elem_id_set_device(l, 0); > > snd_ctl_elem_id_set_device(r, UINT_MAX); > > > > assert(snd_ctl_elem_id_compare(l, r) < 0); > > > > return 0; > > } > > ``` > > > > The assertion hits. For conversion detail: > > > > ``` > > $ cat test1.c > > #include <stdio.h> > > #include <stdlib.h> > > #include <limits.h> > > > > int main() > > { > > unsigned int a, b; > > int diff; > > > > a = 0; > > b = 10; > > diff = a - b; > > printf("%08x\n", diff); > > > > a = 0; > > b = UINT_MAX; > > diff = a - b; > > printf("%08x\n", diff); > > > > return EXIT_SUCCESS; > > } > > ``` > > > > The above test results in 0x00000001 for -UINT_MAX case under x386/x86_64, > > like: > > > > ``` > > $ gcc -m32 -o ./test1 ./test1.c ; ./test1 > > fffffff6 > > 00000001 > > $ gcc -m64 -o ./test1 ./test1.c ; ./test1 > > fffffff6 > > 00000001 > > ``` > > > > We can see integer overflow in both machine architectures due to > > calculation under 32 bit storage. > > > > Well, let us prepare 64 bit storage for both of minuend and subtrahend > > to get negative value in 64 bit storage. In the case, narrower conversion > > to 32 bit integer is unavoidable since it's assigned to integer value > > returned from the function in your implementation. In the case, converted > > value is _not_ negative according to assignment rule in C language > > specification. > > > > ``` > > $ cat test2.c > > #include <stdio.h> > > #include <stdlib.h> > > #include <limits.h> > > > > int main() > > { > > unsigned int a, b; > > long long diff; > > int ret; > > > > a = 0; > > b = UINT_MAX; > > > > // Calculate under 64 bit storage, then assign to 64 bit storage. > > diff = (long long)a - (long long)b; > > printf("%016llx\n", diff); > > > > // Narrower conversion occurs now. > > ret = (int)diff; > > printf("%08x\n", ret); > > > > return EXIT_SUCCESS; > > } > > $ gcc -m32 -o ./test2 ./test2.c ; ./test2 > > ffffffff00000001 > > 00000001 > > $ gcc -m64 -o ./test2 ./test2.c ; ./test2 > > ffffffff00000001 > > 00000001 > > ``` > > > > We can see easy example in the clause of 'Assignment operators' in the > > specification. This is the reason to use condition branches in the patchset. > > The point is that none of the compared unsigned fields is really above the > 31-bit range, so you're trying to resolve an academical problem instead to add > the debug checks (asserts) if the input values are in the acceptable range. > Only the numid functions require this. Hm. I think you have the assumption to 'device' and 'subdevice' fields. If the value of these fields directly derived from any fields systematically which Linux kernel or middleware of ALSA kernel stuffs maintains with 'int' type, it would be valid. However, the decision to assign specific value to these fields is left to driver developer, by declaring 'struct snd_kcontrol_new'[1] in driver code. We wouldn't see such code that the developer construct 'pseudo' device and subdevice to deliver specific information to userspace, it could be. (once I've investigated to use this design to ALSA firewire stack.) Additionally, alsa-lib has plug-in framework to have several backend which works beyond the most of API calls[2]. The 'hw' plugin is one of them, which directly communicate to ALSA control core via system calls. Developers and users have some opportunities to implement and use the other backend, then they are free from your assumption. In this point, any assumption to 'index' field is not better as well. Of course, you can insist the above topics are not practical, something belongs to domains of academical or logical. However, I put safety in the first place, to avoid bugs which expectedly appears in future. I'd like you to take enough care of downstream user's demands. Well, if code revising is not acceptable to you, it's better to add any assertion to check range of value as you mentioned as well as good documentation. In this case, your function is not generic one and should be renamed that it works conditionally. 'snd_ctl_elem_id_compare()' is not acceptable. [1] include/sound/control.h https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/sound/control.h?h=for-next#n41 [2] External Control Plugin SDK https://www.alsa-project.org/alsa-doc/alsa-lib/ctl_external_plugins.html Regards Takashi Sakamoto
Hi Jaroslav, On Thu, Mar 18, 2021 at 06:17:54PM +0100, Jaroslav Kysela wrote: > Dne 18. 03. 21 v 17:56 Takashi Sakamoto napsal(a): > > On Fri, Mar 19, 2021 at 01:37:15AM +0900, Takashi Sakamoto wrote: > >> As I described, your old implementation is not acceptable just by renaming. > >> Although the idea of inline definitions is itself preferable. I suspect whether > >> inline definition for your comparison algorithm is really less overhead than > >> function call. > >> > >> Anyway I'll post revised version of patchset later. > > > > Oops. These APIs have arguments with opaque pointers. In the case, > > inline definition is not available since the layout of structure underlying > > the pointer is not available for userspace applications. Thus the rest of > > issue is whether to use 'tuple' or 'fields' in the name of new API. > > > > In my opinion, 'fields' is generic expression and could give impression to > > application developers that it includes numid field as well. On the other > > hand, 'tuple' is weak expression somehow and the developers easily find > > its meaning in alsa-lib documentation (see line 80). When considering about > > helpfulness to developers, 'tuple' seems to be better than 'fields'. > > With this logic, we can just create snd_ctl_id_compare1, snd_ctl_id_compare2 > functions to force developers to go to the docs. It's not better since it's against common convention for name of exposed API. When you work for internal helper function which is not exposed, it's acceptable. At least, I have never seen such functions in alsa-lib. > Perhaps, snd_ctl_id_compare_full() may be better. Tuple is a generic set of > fields, so there's no change. As I described, the usage of 'tuple' in the context is in documentation. In this case, it's not so bad idea and acceptable I think. At least, it's better than the word 'full' since your comparison algorithm is not based on full fields by ignoring numid field. > Again, I don't expect to add other comparison functions soon. I'd like you to explain about your rejection to add comparison function according to numid as well as your view of the comparison algorithm as being exclusive, single, unique than the others when we have many comparison algorithms for fields of the tuple. Regards Takashi Sakamoto
Hi, This patchset is a fix for bug issued in the message thread[1]. In this development period, alsa-lib got new API as implementation for one of comparison algorithms to a pair of control element IDs. However, it has several issues. At first, the name, 'snd_ctl_elem_id_compare()', is inappropriate since it implements one of comparison algorithms. The name itself implies the algorithm is single and unique for control element ID. However, the target structure, 'struct snd_ctl_elem_id', is hybrid and compound one. We can not find such single and unique comparison algorithm for it. Secondary, it subtracts a pair of values in fields of 'unsigned int' type in storage size of the type. It brings integer overflow. Tertiary, it has simple bug to compare subdevice field in the same structure. Essentially, equality is different from comparison. In a point of programming, implementation for comparison algorithm can have more overhead than implementation for equality. In this meaning, it's better to add different API for them. This patchset adds new API below: * for equality * snd_ctl_elem_id_equal_by_numid() * snd_ctl_elem_id_equal_by_tuple() * for each comparison algorithm * snd_ctl_elem_id_compare_by_numid() * snd_ctl_elem_id_compare_by_tuple_arithmetic() I've got bothered to decide the name of API for the case to use tuples. Here I use the word, 'tuple', which comes from documentation of alsa-lib[2]. Furthermore, this patchset adds test program for them since equality and comparison are quite basic method to operate data. It's better to have no bug. Finally, the issued API, 'snd_ctl_elem_id_compare()' is dropped. After merging the patchset, I'm going to post additional patch to alsa-utils to fix issued line[3]. [1] https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/181738.html [2] https://github.com/alsa-project/alsa-lib/blob/master/src/control/control.c#L80 [3] https://github.com/alsa-project/alsa-utils/blob/master/alsactl/clean.c#L55 Regards Takashi Sakamoto (6): test: ctl-elem-id: add test program for future APIs relevant to control element ID ctl: add API to check equality between a pair of control element IDs by numid ctl: add API to check equality between a pair of control element IDs by tuple ctl: add API to compare a pair of control element IDs by numid ctl: add API to compare a pair of control element IDs by one of algorithms according to tuple ctl: drop deprecated API to compare a pair of control element IDs include/control.h | 5 +- src/control/control.c | 135 ++++++++++++++---- test/lsb/Makefile.am | 6 +- test/lsb/ctl-elem-id.c | 301 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 418 insertions(+), 29 deletions(-) create mode 100644 test/lsb/ctl-elem-id.c