[03/13] Installed-header hygiene (BZ#20366): missing inclusions A.
Commit Message
These are all fallout from the change from u_intNN_t to uintNN_t; a
number of headers now need to include <stdint.h> to pick up those
types. It is possible that __uintNN_t should be used instead.
Some of these files directly included <features.h> and/or
<sys/cdefs.h>, which I removed, as the style generally seems to be to
let <sys/types.h> do that for us. One file included <asm/types.h>
gratuitously.
* inet/protocols/talkd.h, resolv/arpa/nameser.h
* sysdeps/generic/netinet/in_systm.h
* sysdeps/gnu/netinet/ip_icmp.h, sysdeps/gnu/netinet/tcp.h
* sysdeps/gnu/netinet/udp.h
* sysdeps/unix/sysv/linux/net/ethernet.h
* sysdeps/unix/sysv/linux/net/if_arp.h
* sysdeps/unix/sysv/linux/net/if_ppp.h
* sysdeps/unix/sysv/linux/net/if_shaper.h
* sysdeps/unix/sysv/linux/netinet/if_fddi.h
* sysdeps/unix/sysv/linux/netinet/if_tr.h
* sysdeps/unix/sysv/linux/netipx/ipx.h
* sysdeps/unix/sysv/linux/sys/acct.h
* sysdeps/unix/sysv/linux/sys/quota.h:
Include stdint.h for uintNN_t definitions.
Don't include sys/cdefs.h, features.h, or asm/types.h directly.
---
inet/protocols/talkd.h | 1 +
resolv/arpa/nameser.h | 2 +-
sysdeps/generic/netinet/in_systm.h | 2 +-
sysdeps/gnu/netinet/ip_icmp.h | 2 +-
sysdeps/gnu/netinet/tcp.h | 1 +
sysdeps/gnu/netinet/udp.h | 3 +--
sysdeps/unix/sysv/linux/net/ethernet.h | 3 ++-
sysdeps/unix/sysv/linux/net/if_arp.h | 3 +--
sysdeps/unix/sysv/linux/net/if_ppp.h | 3 +--
sysdeps/unix/sysv/linux/net/if_shaper.h | 2 +-
sysdeps/unix/sysv/linux/netinet/if_fddi.h | 4 +---
sysdeps/unix/sysv/linux/netinet/if_tr.h | 2 +-
sysdeps/unix/sysv/linux/netipx/ipx.h | 3 +--
sysdeps/unix/sysv/linux/sys/acct.h | 5 ++---
sysdeps/unix/sysv/linux/sys/quota.h | 2 +-
15 files changed, 17 insertions(+), 21 deletions(-)
Comments
On Mon, 29 Aug 2016, Zack Weinberg wrote:
> These are all fallout from the change from u_intNN_t to uintNN_t; a
> number of headers now need to include <stdint.h> to pick up those
> types. It is possible that __uintNN_t should be used instead.
I'm not clear whether this is needed to restore the glibc build /
testsuite build, or only for installed use. If it's needed to keep things
building, it has to come *before* the patch that causes the header to be
required, to keep bisectability.
On Mon, Aug 29, 2016 at 9:23 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 29 Aug 2016, Zack Weinberg wrote:
>
>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>> number of headers now need to include <stdint.h> to pick up those
>> types. It is possible that __uintNN_t should be used instead.
>
> I'm not clear whether this is needed to restore the glibc build /
> testsuite build, or only for installed use. If it's needed to keep things
> building, it has to come *before* the patch that causes the header to be
> required, to keep bisectability.
It is necessary to restore the build. This is only separate for ease
of review; my plan was always to squash the entire series into one big
patch for landing. Or maybe two or three big patches - the time.h and
ucontext.h changes qualify as separate ideas, probably.
zw
On 08/29/2016 09:16 PM, Zack Weinberg wrote:
> These are all fallout from the change from u_intNN_t to uintNN_t; a
> number of headers now need to include <stdint.h> to pick up those
> types. It is possible that __uintNN_t should be used instead.
Why should __uintNN_t ever be used? At a minimum the uintNN_t types
should be available regardless of whatever mode you are being compiled in?
> Some of these files directly included <features.h> and/or
> <sys/cdefs.h>, which I removed, as the style generally seems to be to
> let <sys/types.h> do that for us. One file included <asm/types.h>
> gratuitously.
Are you certain that was safe? I would be worried about arbitrarily
removing those headers without a deeper analysis.
> * inet/protocols/talkd.h, resolv/arpa/nameser.h
> * sysdeps/generic/netinet/in_systm.h
> * sysdeps/gnu/netinet/ip_icmp.h, sysdeps/gnu/netinet/tcp.h
> * sysdeps/gnu/netinet/udp.h
> * sysdeps/unix/sysv/linux/net/ethernet.h
> * sysdeps/unix/sysv/linux/net/if_arp.h
> * sysdeps/unix/sysv/linux/net/if_ppp.h
> * sysdeps/unix/sysv/linux/net/if_shaper.h
> * sysdeps/unix/sysv/linux/netinet/if_fddi.h
> * sysdeps/unix/sysv/linux/netinet/if_tr.h
> * sysdeps/unix/sysv/linux/netipx/ipx.h
> * sysdeps/unix/sysv/linux/sys/acct.h
> * sysdeps/unix/sysv/linux/sys/quota.h:
> Include stdint.h for uintNN_t definitions.
> Don't include sys/cdefs.h, features.h, or asm/types.h directly.
OK only if you verified that the generated code did not change due to the
removal of unneeded headers, or if you drop the unneeded header removal.
On 08/29/2016 11:11 PM, Zack Weinberg wrote:
> On Mon, Aug 29, 2016 at 9:23 PM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Mon, 29 Aug 2016, Zack Weinberg wrote:
>>
>>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>>> number of headers now need to include <stdint.h> to pick up those
>>> types. It is possible that __uintNN_t should be used instead.
>>
>> I'm not clear whether this is needed to restore the glibc build /
>> testsuite build, or only for installed use. If it's needed to keep things
>> building, it has to come *before* the patch that causes the header to be
>> required, to keep bisectability.
>
> It is necessary to restore the build. This is only separate for ease
> of review; my plan was always to squash the entire series into one big
> patch for landing. Or maybe two or three big patches - the time.h and
> ucontext.h changes qualify as separate ideas, probably.
That was my understanding. That this is going to be a single patch that does
the mechanical changes in one commit which can be reverted, as suggested in
patch 00/13.
On Wed, 21 Sep 2016, Carlos O'Donell wrote:
> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
> > These are all fallout from the change from u_intNN_t to uintNN_t; a
> > number of headers now need to include <stdint.h> to pick up those
> > types. It is possible that __uintNN_t should be used instead.
>
> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
> should be available regardless of whatever mode you are being compiled in?
ISO C doesn't reserve *_t globally, but unless any of these headers are
ISO C headers that's not a relevant consideration (and if inappropriate
*_t types were being added to ISO C headers that would have been detected
by the conform/ tests).
On Wed, Sep 21, 2016 at 1:52 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>> number of headers now need to include <stdint.h> to pick up those
>> types. It is possible that __uintNN_t should be used instead.
>
> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
> should be available regardless of whatever mode you are being compiled in?
It might be inappropriate for some of these headers to (behave as-if
they) include <stdint.h>. Joseph says that existing conform/ tests
cover this from a standards-compliance perspective, but it is my
understanding that net/* and netinet/* headers are not very well
standardized, so we might want to think about whether we should go to
extra effort here.
>> Some of these files directly included <features.h> and/or
>> <sys/cdefs.h>, which I removed, as the style generally seems to be to
>> let <sys/types.h> do that for us. One file included <asm/types.h>
>> gratuitously.
>
> Are you certain that was safe? I would be worried about arbitrarily
> removing those headers without a deeper analysis.
sys/types.h unconditionally includes features.h, and features.h
unconditionally includes sys/cdefs.h.
None of the definitions in asm/types.h were required by the file that
was including it.
I will do a check for unchanged installed libraries as discussed elsewhere.
zw
On 09/21/2016 02:22 PM, Zack Weinberg wrote:
> On Wed, Sep 21, 2016 at 1:52 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
>>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>>> number of headers now need to include <stdint.h> to pick up those
>>> types. It is possible that __uintNN_t should be used instead.
>>
>> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
>> should be available regardless of whatever mode you are being compiled in?
>
> It might be inappropriate for some of these headers to (behave as-if
> they) include <stdint.h>. Joseph says that existing conform/ tests
> cover this from a standards-compliance perspective, but it is my
> understanding that net/* and netinet/* headers are not very well
> standardized, so we might want to think about whether we should go to
> extra effort here.
Thanks for the clarification.
Given the underspecified nature of those headers I don't see that we need
to worry. If someone comes back and has a valid complaint we could always
do a further cleanup?
>>> Some of these files directly included <features.h> and/or
>>> <sys/cdefs.h>, which I removed, as the style generally seems to be to
>>> let <sys/types.h> do that for us. One file included <asm/types.h>
>>> gratuitously.
>>
>> Are you certain that was safe? I would be worried about arbitrarily
>> removing those headers without a deeper analysis.
>
> sys/types.h unconditionally includes features.h, and features.h
> unconditionally includes sys/cdefs.h.
>
> None of the definitions in asm/types.h were required by the file that
> was including it.
>
> I will do a check for unchanged installed libraries as discussed elsewhere.
Belt-and-suspenders ;-)
@@ -52,6 +52,7 @@
#include <sys/types.h>
#include <sys/socket.h>
+#include <stdint.h>
/*
* Client->server request message format.
@@ -58,7 +58,7 @@
#include <sys/param.h>
#include <sys/types.h>
-#include <sys/cdefs.h>
+#include <stdint.h>
/*%
* Revision information. This is the release date in YYYYMMDD format.
@@ -19,8 +19,8 @@
#ifndef _NETINET_IN_SYSTM_H
#define _NETINET_IN_SYSTM_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
+#include <stdint.h>
__BEGIN_DECLS
@@ -18,8 +18,8 @@
#ifndef __NETINET_IP_ICMP_H
#define __NETINET_IP_ICMP_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
+#include <stdint.h>
__BEGIN_DECLS
@@ -73,6 +73,7 @@
#ifdef __USE_MISC
# include <sys/types.h>
# include <sys/socket.h>
+# include <stdint.h>
typedef uint32_t tcp_seq;
/*
@@ -47,9 +47,8 @@
#ifndef __NETINET_UDP_H
#define __NETINET_UDP_H 1
-#include <features.h>
#include <sys/types.h>
-
+#include <stdint.h>
/* UDP header as specified by RFC 768, August 1980. */
@@ -21,8 +21,9 @@
#ifndef __NET_ETHERNET_H
#define __NET_ETHERNET_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
+#include <stdint.h>
+
#include <linux/if_ether.h> /* IEEE 802.3 Ethernet constants */
__BEGIN_DECLS
@@ -20,12 +20,11 @@
/* Based on the 4.4BSD and Linux version of this file. */
#ifndef _NET_IF_ARP_H
-
#define _NET_IF_ARP_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
#include <sys/socket.h>
+#include <stdint.h>
__BEGIN_DECLS
@@ -49,8 +49,7 @@
#define __NET_IF_PPP_H 1
#include <sys/types.h>
-#include <sys/cdefs.h>
-
+#include <stdint.h>
#include <net/if.h>
#include <sys/ioctl.h>
#include <net/ppp_defs.h>
@@ -18,8 +18,8 @@
#ifndef _NET_IF_SHAPER_H
#define _NET_IF_SHAPER_H 1
-#include <features.h>
#include <sys/types.h>
+#include <stdint.h>
#include <net/if.h>
#include <sys/ioctl.h>
@@ -18,10 +18,8 @@
#ifndef _NETINET_IF_FDDI_H
#define _NETINET_IF_FDDI_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
-#include <asm/types.h>
-
+#include <stdint.h>
#include <linux/if_fddi.h>
#ifdef __USE_MISC
@@ -18,8 +18,8 @@
#ifndef _NETINET_IF_TR_H
#define _NETINET_IF_TR_H 1
-#include <sys/cdefs.h>
#include <sys/types.h>
+#include <stdint.h>
/* IEEE 802.5 Token-Ring magic constants. The frame sizes omit the preamble
and FCS/CRC (frame check sequence). */
@@ -18,9 +18,8 @@
#ifndef __NETIPX_IPX_H
#define __NETIPX_IPX_H 1
-#include <features.h>
-
#include <sys/types.h>
+#include <stdint.h>
#include <bits/sockaddr.h>
__BEGIN_DECLS
@@ -18,12 +18,11 @@
#ifndef _SYS_ACCT_H
#define _SYS_ACCT_H 1
-#include <features.h>
-
+#include <sys/types.h>
+#include <stdint.h>
#include <endian.h>
#define __need_time_t
#include <time.h>
-#include <sys/types.h>
__BEGIN_DECLS
@@ -35,8 +35,8 @@
#ifndef _SYS_QUOTA_H
#define _SYS_QUOTA_H 1
-#include <features.h>
#include <sys/types.h>
+#include <stdint.h>
/*
* Select between different incompatible quota versions.