[PATCH rpcbind] src: include cdefs.h for the __P() macro

[PATCH rpcbind] src: include cdefs.h for the __P() macro

[Yann E. MORIN]

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(+)

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

Re: [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Chuck Lever]

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

Re: [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Chuck Lever]

[ 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

Re: [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [Libtirpc-devel] [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Steve Dickson]

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
> 
> 
> 

Re: [Libtirpc-devel] [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [Libtirpc-devel] [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Steve Dickson]

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.
> 

Re: [Libtirpc-devel] [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [Libtirpc-devel] [PATCH rpcbind] src: include cdefs.h for the __P() macro

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]