On 08/15/2016 11:48 AM, Yann E. MORIN wrote: > Steve, All, > > On 2016-08-15 11:16 -0400, Steve Dickson spake thusly: >> On 08/15/2016 10:23 AM, Chuck Lever wrote: >>>> On Aug 14, 2016, at 6:13 PM, Yann E. MORIN <yann.morin.1998@xxxxxxx> wrote: >>>> On 2016-08-14 14:30 -0400, Chuck Lever spake thusly: >>>>>> On Aug 13, 2016, at 10:05 AM, Yann E. MORIN <yann.morin.1998@xxxxxxx> wrote: >>>>>> >>>>>> The __P() macro is defined in cdefs.h, so we must include it explicitly >>>>>> rather than relying on it being included by another header. >>>>>> >>>>>> cdefs.h is a glibc-ism; glibc includes it almost everywhere from its own >>>>>> headers. So it automatically gets included for glibc. >>>>>> >>>>>> However, cdefs.h is not present in musl, so its headers do not include >>>>>> it. We must thus include it when we need __P() (of course, one will have >>>>>> to provide his own cdefs.h in this case). >>>>> >>>>> Simply adding "#include <sys/cdefs.h>" seems like the wrong approach. >>>>> If cdefs.h is not guaranteed to exist, the appropriate thing to do >>>>> is provide some autoconf machinery to define __P() in its absence. >>>> >>>> OpenEmbedded provides comaptibility headers: >>>> http://git.openembedded.org/openembedded-core/tree/meta/recipes-core/bsd-headers/bsd-headers >>>> >>>> In Buildroot, we're adding them too (not yet applied, WIP): >>>> http://lists.busybox.net/pipermail/buildroot/2016-August/169722.html >>>> >>>> Other embedded buildsystem may each have their own fix in a way or >>>> another... >>>> >>>> Mainstream distros are more-or-less all based on glibc, except for a few >>>> outliers, like Alpine Linux (also based on musl), and they've gone on >>>> the "remove __P()" route: >>>> http://git.alpinelinux.org/cgit/aports/tree/main/rpcbind/0001-Avoid-use-of-glibc-sys-cdefs.h-header.patch >>>> >>>>> On the other hand, I wonder if we need to continue to preserve K&R C >>>>> compatibility in this code base. Perhaps instead the uses of __P() >>>>> should be eliminated? >>>> >>>> I tried to provide a minimalist approach, that consists in assuming that >>>> cdefs.h is present. >>> >>> If cdefs.h presence cannot be guaranteed (and I think you've adequately >>> demonstrated that no guarantee exists), at the very least there needs >>> to be some autoconf logic to handle the "cdefs.h is not present" case. >>> IMO a strictly minimalist approach won't work here. >> I don't see how it *can't* exist... At lease with Fedora and RHEL >> cdefs.h is part of the glibc-headers rpm and without that nothing >> in nfs-utils is going to compile > > Well, not everything is using glibc; there are other C libraries in the > world. Not everything is Fedora or RHEL either; there are other distros > out there. Not everything is a distro either; there are embedded systems > that use custom buildsystems. Fair enough... > > musl is a strict standard-compliant C library; it does not implement > anything not in a standard. sys/cdefs.h is defined in no standard, thus > not provided by musl. > > http://www.musl-libc.org/ > > Furthermore, nothing in nfs-utils uses sys/cdefs.h or the usual culprit > macros defined in there: > > $ git grep -E 'cdefs\.h|__P|_DECLS' > [nothing] > > For the record, in Buildroot we do build nfs-utils on musl without any > issue. This is good to hear... > >>>> But I do agree that pre-ANSI compatibility is probably a little tiny >>>> wee bit excessive nowadays. Virtually all current compilers do accept >>>> function prototypes, nowadays... >>>> >>>> I can work on a patch that does just get rid of the use of __P(). (we >>>> can't really vampirise the patch from Alpine, as there's no SoB or such >>>> origin information on it; not that redoing the patch would be too >>>> difficult either...). >>>> >>>> So, what route, now? ;-) >>> >>> My preference as a reviewer and individual contributor: >>> >>> Barring any further comments here, provide two different approaches: >>> >>> 1. add autoconf logic to detect when sys/cdefs.h is not available, >>> and provide a substitute __P() macro. That might be as simple as >>> defining __P in a local auto.m4 script when it is not provided by >>> system headers. >> I thinking we should fail the configuration if sys/cdefs.h does not >> exist... > > And thus break on systems that do not use glibc (or uClibc)? Point. > >>> 2. remove invocations of the __P() macro from the rpcbind source >> Any idea what could break by removing them?? > > Virtually nothing. If you look at the glibc code, __P(arg) just expands > to its argument arg: > > /* These two macros are not used in glibc anymore. They are kept here > only because some other projects expect the macros to be defined. */ > #define __P(args) args > #define __PMT(args) args > > Note that Chuck and I are talking about removing the use of the __P() > macro, not about removing the prototypes. E.g., transform such code: > > int f __P((inti )); > > into: > > int f(int i); > > which is what happens anyway with the __P() macro. hmm... it just worries me to remove things from code that is this aged ;-) 9 out 10 times something break. > >>> Post both to the mailing lists and folks here can decide which is >>> better. >>> >>> You might not have time for all that ;-) so you could pick one and >>> add a strong technical argument in the patch description why that >>> is the best choice. >>> >>> I think I like 2. overall as it should leave the rpcbind source >>> code a little easier to read, no new autoconf logic is needed, and >>> there appears to be one distro that is already going that way. >> I lean more toward taking the patch as is and failing the >> configuration if the header file does not exist.. > > Still the case with the above explanations? I do see your points... It still worries me but lets hope nothing breaks when they are removed... steved. > > Regards, > Yann E. MORIN. > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
