Re: [Acl-devel] [RFC] Add attribute visibility("default") to API functions

[Yury Usishchev]

Hello Mike!

I did not fully understand your idea last time, sorry:)
I get it now, and yes, it can work, but it will need some more changes to 
sources.

While 'include/acl.h' is working well with new approach, I have problems with 
declarations from 'include/libacl.h'.
Last header is also public, and it is not included into source files during 
build. So now I'm getting errors like

acl/build/../tools/chacl.c:275: undefined reference to `acl_entries'

Also there is an error in tools/parse.c:
here config.h in included after acl.h:

In file included from ../include/misc.h:21:0,
from ../tools/parse.c:37:
./include/config.h:5:0: warning: "ACL_API_EXPORT" redefined
#define ACL_API_EXPORT __attribute__ ((visibility ("default"))) extern
In file included from ../tools/parse.c:33:0:
./include/sys/acl.h:32:0: note: this is the location of the previous definition
# define ACL_API_EXPORT extern

Second problem is easy to fix - just move includes in tools/parse.c
But first one will require changes to many files in libacl/ directory

For example - libacl/acl_check.c
It includes libacl/libacl.h which includes include/acl.h. From build command 
comes include libacl/perm_copy.h which
includes misc.h (with config.h)
However it does not include include/libacl.h where acl_check is declared (with 
proper visibility attribute) therefore
acl_check is not visible.

I can name at least 2 more such files(libacl/acl_entries.c and 
libacl/acl_get_perm.c).

Should I fix these problems? And if yes should that fix go as separate patch or 
same is ok?

I attached new patch - you can check build failures yourself. I used following 
configure options:
CFLAGS="-O2 -g -flto -fvisibility=hidden" AR=gcc-ar RANLIB=gcc-ranlib 
../configure

BR,
Yury Usishchev

0001-add-visibility-default-for-api-functions.patch
Description: patch

Mike Frysinger <address@hidden> writes:

> On 12 Feb 2016 16:39, Yury Usishchev wrote:
>> > (2) just do in acl.h:
>> > #ifndef ACL_API_EXPORT
>> > # define ACL_API_EXPORT extern
>> > #endif
>> > (3) update config.h to define ACL_API_EXPORT to default visiblity
>> > (4) update configure.ac to test for the flag directly -- we already have
>> > the m4/visibility_hidden.m4 file in the codebase for this
>> >
>> > since all our source files should be including config.h first, we
>> > then know the macro will be defined before pulling in acl.h.
>>
>> It will be better if we put it in include/misc.h like the hidden macros
>> And I dont think we need to add another check for visibility("default")
>> as it should be covered by visibility("hidden") check.
>> 
>> > there's no need to enumerate the definitions themselves.  just the
>> > prototype is sufficient.
>>
>> As Andreas pointed out, information about visibility cannot be used outside
>> acl build, so we should not place it in public headers. Thats why I annotated
>> definitions.
>
> my version didn't include visibility.  the public header *only* has the
> three lines i posted earlier:
> #ifndef ACL_API_EXPORT
> # define ACL_API_EXPORT extern
> #endif
>
> that's the point of putting the define into config.h.
> -mike