| Message ID | 1471097125-13193-1-git-send-email-yann.morin.1998@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Yann- > On Aug 13, 2016, at 10:05 AM, Yann E. MORIN <yann.morin.1998@free.fr> 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. 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? > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > src/check_bound.c | 1 + > src/pmap_svc.c | 1 + > src/rpcb_svc.c | 1 + > src/rpcb_svc_4.c | 1 + > src/rpcb_svc_com.c | 1 + > src/rpcbind.c | 1 + > src/util.c | 1 + > src/warmstart.c | 1 + > 8 files changed, 8 insertions(+) > > diff --git a/src/check_bound.c b/src/check_bound.c > index c70b845..df14fbc 100644 > --- a/src/check_bound.c > +++ b/src/check_bound.c > @@ -49,6 +49,7 @@ static char sccsid[] = "@(#)check_bound.c 1.11 89/04/21 Copyr 1989 Sun Micro"; > > #include <sys/types.h> > #include <sys/socket.h> > +#include <sys/cdefs.h> > #include <rpc/rpc.h> > #include <stdio.h> > #include <netconfig.h> > diff --git a/src/pmap_svc.c b/src/pmap_svc.c > index ad28b93..5993c0b 100644 > --- a/src/pmap_svc.c > +++ b/src/pmap_svc.c > @@ -49,6 +49,7 @@ static char sccsid[] = "@(#)pmap_svc.c 1.23 89/04/05 Copyr 1984 Sun Micro"; > #ifdef PORTMAP > #include <sys/types.h> > #include <sys/socket.h> > +#include <sys/cdefs.h> > #include <stdio.h> > #include <rpc/rpc.h> > #include <rpc/pmap_prot.h> > diff --git a/src/rpcb_svc.c b/src/rpcb_svc.c > index bd92201..eb5b49c 100644 > --- a/src/rpcb_svc.c > +++ b/src/rpcb_svc.c > @@ -42,6 +42,7 @@ > * version 3 of rpcbind. > */ > #include <sys/types.h> > +#include <sys/cdefs.h> > #include <rpc/rpc.h> > #include <rpc/rpcb_prot.h> > #include <netconfig.h> > diff --git a/src/rpcb_svc_4.c b/src/rpcb_svc_4.c > index b673452..69d75e3 100644 > --- a/src/rpcb_svc_4.c > +++ b/src/rpcb_svc_4.c > @@ -44,6 +44,7 @@ > > #include <sys/types.h> > #include <sys/stat.h> > +#include <sys/cdefs.h> > #include <rpc/rpc.h> > #include <stdio.h> > #include <unistd.h> > diff --git a/src/rpcb_svc_com.c b/src/rpcb_svc_com.c > index 148fe42..9369e27 100644 > --- a/src/rpcb_svc_com.c > +++ b/src/rpcb_svc_com.c > @@ -43,6 +43,7 @@ > #include <sys/stat.h> > #include <sys/param.h> > #include <sys/poll.h> > +#include <sys/cdefs.h> > #include <bits/poll.h> > #include <sys/socket.h> > #include <rpc/rpc.h> > diff --git a/src/rpcbind.c b/src/rpcbind.c > index c4265cd..37fc660 100644 > --- a/src/rpcbind.c > +++ b/src/rpcbind.c > @@ -50,6 +50,7 @@ > #include <sys/file.h> > #include <sys/socket.h> > #include <sys/un.h> > +#include <sys/cdefs.h> > #include <netinet/in.h> > #include <rpc/rpc.h> > #include <rpc/rpc_com.h> > diff --git a/src/util.c b/src/util.c > index a6c835b..5bbec1d 100644 > --- a/src/util.c > +++ b/src/util.c > @@ -42,6 +42,7 @@ > #include <sys/types.h> > #include <sys/socket.h> > #include <sys/queue.h> > +#include <sys/cdefs.h> > #include <net/if.h> > #include <netinet/in.h> > #include <ifaddrs.h> > diff --git a/src/warmstart.c b/src/warmstart.c > index b6eb73e..f3a9c29 100644 > --- a/src/warmstart.c > +++ b/src/warmstart.c > @@ -34,6 +34,7 @@ > > #include <sys/types.h> > #include <sys/stat.h> > +#include <sys/cdefs.h> > #include <stdio.h> > #include <rpc/rpc.h> > #include <rpc/rpcb_prot.h> > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chuck, All, 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@free.fr> 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. 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? ;-) Regards, Yann E. MORIN.
[ adding libtirpc-devel ] > On Aug 14, 2016, at 6:13 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Chuck, All, > > 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@free.fr> 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. > 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. 2. remove invocations of the __P() macro from the rpcbind source 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. Maybe there's someone with the Alpine distro that can provide an SoB for their patch? -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chuck, All, On 2016-08-15 10:23 -0400, Chuck Lever spake thusly: > > On Aug 14, 2016, at 6:13 PM, Yann E. MORIN <yann.morin.1998@free.fr> 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@free.fr> 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. > > > 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. > > 2. remove invocations of the __P() macro from the rpcbind source > > 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. Indeed, I don't have time for both. I'm going for 2. as described above, because it is what technically makes sense: keeping this legacy pre-ANSI support is useless, and adding even more compatibility checks for it is even more useless (IMHO). I'll wait a bit for more comments before working on a patch... > Maybe there's someone with the Alpine distro that can provide an > SoB for their patch? I won't say it's impossible, but already tried for other cases and... Nope... :-/ Thanks for the feedback! :-) Regards, Yann E. MORIN.
On 08/15/2016 10:23 AM, Chuck Lever wrote: > [ adding libtirpc-devel ] > > >> On Aug 14, 2016, at 6:13 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: >> >> Chuck, All, >> >> 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@free.fr> 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 > > >> 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... > > 2. remove invocations of the __P() macro from the rpcbind source Any idea what could break by removing them?? > > 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.. I see how anything could break with that approach. steved. > > Maybe there's someone with the Alpine distro that can provide an > SoB for their patch? > > > -- > Chuck Lever > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@free.fr> 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@free.fr> 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. 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. > >> 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)? > > 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. > > 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? Regards, Yann E. MORIN.
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@free.fr> 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@free.fr> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Steve, All, Thanks for the feedback! :-) On 2016-08-15 12:30 -0400, Steve Dickson spake thusly: > On 08/15/2016 11:48 AM, Yann E. MORIN wrote: > > On 2016-08-15 11:16 -0400, Steve Dickson spake thusly: [--SNIP--] > >> 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 [--SNIP--] > hmm... it just worries me to remove things from code that > is this aged ;-) 9 out 10 times something break. Yes, I understand your position. "If it aint' broke, don't fix it." But I argue it is broken. ;-) Just to expand on my position, I did some more research. glibc has dropped K&R support since 2000 (16 yeas ago!): commit 7f4e0e588681d0670c78472f81c21e63bb5772d6 Author: Ulrich Drepper <drepper@redhat.com> Date: Fri Mar 31 04:17:54 2000 +0000 Update. 2000-03-30 Andreas Jaeger <aj@suse.de> * misc/sys/cdefs.h: Remove K&R support. And since then, __P() has been defined to just expand to its argument. The current code was commited in 2004, in d19687d6 (which touches a lot of files, unfortunately, so not reproducing it here...) So, trying a little thought experiment now. All versions of gcc that we are supposed to encounter nowadays are ANSI-compliant; they don;t need __P(). The other compiler worth investigating is clang. I haven't checked, but I would expect it is ANSI-compliant too. Just as a joke, tinycc claims to be C99, so ANSI-compliant as well. We're left with obscur or commercial compilers now. Which one would not be ANSI-compliant and would still use K&R rules? If that was the case, they would not be able to build even 1% of the corpus of open source code available. And even if such a compiler existed, would we want to support it? > >>> 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... So, does that mean you're OK with a patch to get rid of them (at least to review it)? Regards, Yann E. MORIN.
GNU projects have been dropping the __P cruft for a while now, and that includes autoconf and gnulib, both of which are more or less the "gold" standard when it comes to portability. -mike
diff --git a/src/check_bound.c b/src/check_bound.c index c70b845..df14fbc 100644 --- a/src/check_bound.c +++ b/src/check_bound.c @@ -49,6 +49,7 @@ static char sccsid[] = "@(#)check_bound.c 1.11 89/04/21 Copyr 1989 Sun Micro"; #include <sys/types.h> #include <sys/socket.h> +#include <sys/cdefs.h> #include <rpc/rpc.h> #include <stdio.h> #include <netconfig.h> diff --git a/src/pmap_svc.c b/src/pmap_svc.c index ad28b93..5993c0b 100644 --- a/src/pmap_svc.c +++ b/src/pmap_svc.c @@ -49,6 +49,7 @@ static char sccsid[] = "@(#)pmap_svc.c 1.23 89/04/05 Copyr 1984 Sun Micro"; #ifdef PORTMAP #include <sys/types.h> #include <sys/socket.h> +#include <sys/cdefs.h> #include <stdio.h> #include <rpc/rpc.h> #include <rpc/pmap_prot.h> diff --git a/src/rpcb_svc.c b/src/rpcb_svc.c index bd92201..eb5b49c 100644 --- a/src/rpcb_svc.c +++ b/src/rpcb_svc.c @@ -42,6 +42,7 @@ * version 3 of rpcbind. */ #include <sys/types.h> +#include <sys/cdefs.h> #include <rpc/rpc.h> #include <rpc/rpcb_prot.h> #include <netconfig.h> diff --git a/src/rpcb_svc_4.c b/src/rpcb_svc_4.c index b673452..69d75e3 100644 --- a/src/rpcb_svc_4.c +++ b/src/rpcb_svc_4.c @@ -44,6 +44,7 @@ #include <sys/types.h> #include <sys/stat.h> +#include <sys/cdefs.h> #include <rpc/rpc.h> #include <stdio.h> #include <unistd.h> diff --git a/src/rpcb_svc_com.c b/src/rpcb_svc_com.c index 148fe42..9369e27 100644 --- a/src/rpcb_svc_com.c +++ b/src/rpcb_svc_com.c @@ -43,6 +43,7 @@ #include <sys/stat.h> #include <sys/param.h> #include <sys/poll.h> +#include <sys/cdefs.h> #include <bits/poll.h> #include <sys/socket.h> #include <rpc/rpc.h> diff --git a/src/rpcbind.c b/src/rpcbind.c index c4265cd..37fc660 100644 --- a/src/rpcbind.c +++ b/src/rpcbind.c @@ -50,6 +50,7 @@ #include <sys/file.h> #include <sys/socket.h> #include <sys/un.h> +#include <sys/cdefs.h> #include <netinet/in.h> #include <rpc/rpc.h> #include <rpc/rpc_com.h> diff --git a/src/util.c b/src/util.c index a6c835b..5bbec1d 100644 --- a/src/util.c +++ b/src/util.c @@ -42,6 +42,7 @@ #include <sys/types.h> #include <sys/socket.h> #include <sys/queue.h> +#include <sys/cdefs.h> #include <net/if.h> #include <netinet/in.h> #include <ifaddrs.h> diff --git a/src/warmstart.c b/src/warmstart.c index b6eb73e..f3a9c29 100644 --- a/src/warmstart.c +++ b/src/warmstart.c @@ -34,6 +34,7 @@ #include <sys/types.h> #include <sys/stat.h> +#include <sys/cdefs.h> #include <stdio.h> #include <rpc/rpc.h> #include <rpc/rpcb_prot.h>
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). Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- src/check_bound.c | 1 + src/pmap_svc.c | 1 + src/rpcb_svc.c | 1 + src/rpcb_svc_4.c | 1 + src/rpcb_svc_com.c | 1 + src/rpcbind.c | 1 + src/util.c | 1 + src/warmstart.c | 1 + 8 files changed, 8 insertions(+)