Re: [RFC][PATCH 0/2] ALSA: control: export all of TLV related macros to user land

[Takashi Iwai <tiwai@suse.de>]

On Sun, 11 Sep 2016 05:06:41 +0200,
Takashi Sakamoto wrote:
> 
> On Sep 10 2016 22:41, Takashi Iwai wrote:
> > On Sat, 10 Sep 2016 09:25:31 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Sep 10 2016 15:44, Takashi Iwai wrote:
> >>> On Sat, 10 Sep 2016 06:50:14 +0200,
> >>> Takashi Sakamoto wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> Currently, TLV related protocol is not shared to user land. This is not
> >>>> good in a point of application interfaces, because application developers
> >>>> can't realize the protocol just to see UAPI headers.
> >>>>
> >>>> For this purpose, this patchset moves all of macros related to TLV to UAPI
> >>>> header. As a result, a header just for kernel land is obsoleted. When adding
> >>>> new items to the protocol, it's added to the UAPI header. This change affects
> >>>> some drivers in media subsystem.
> >>>>
> >>>> In my concern, this change can break applications. When these macros are
> >>>> already defined in application side and they includes tlv UAPI header
> >>>> directly, 'redefined' warning is generated at preprocess time. But the
> >>>> compilation will be success itself. If these two macros have different
> >>>> content, the result of preprocess is dominated to the order to define.
> >>>> However, the most applications are assumed to use TLV feature via libraries
> >>>> such as alsa-lib, thus I'm optimistic to this concern.
> >>>>
> >>>> As another my concern, the name of these macros are quite simple, as
> >>>> 'TLV_XXX'. It might be help application developers to rename them with a
> >>>> prefix, as 'SNDRV_CTL_TLV_XXX'. (But not yet. I'm a lazy guy.)
> >>>
> >>> The second patch does simply wrong.  You must not obsolete
> >>> include/sound/tlv.h.  Even if it includes only uapi/*, it should be
> >>> still there.
> >>
> >> Any reasons?
> > 
> > The concept and the design.
> > 
> > Don't need to change the root inclusion, it's just to provide cleaner
> > uapi header files, and not meant to be included directly -- it was the
> > basic idea when uapi split was introduced.
> 
> OK. I can see what you indicated in this post for UAPI idea[0]. I'm
> ready to drop the second patch.
> 
> Well, how do you think about my concern of macro prefix? For example, we
> can apply below step:
> 
> Put substantial macros with renaming to 'include/uapi.sound/tlv.h':
> #define SNDRV_CTL_TLV_DATA_LENGTH(...) \
>        ((unsigned int)sizeof((const unsigned int[]) { __VA_ARGS__ }))
> 
> Then, put alias macros to 'include/sound/tlv.h':
> #include <uapi/sound/tlv.h>
> #define TLV_LENGTH SNDRV_CTL_TLV_DATA_LENGTH
> ...
> 
> Finally, applications can expand these macro with apparent names with
> prefix of 'SNDRV_CTL_TLV_DATA_XXX'. I think the prefix prevent
> application codes from name conflict by including 'uapi/sound/tlv.h'.

Yes, that would work.


Takashi