[06/10] Remove __need_list_t and __need_res_state.

Message ID 20170509154103.11973-7-zackw@panix.com
State Superseded
Headers

Commit Message

Zack Weinberg May 9, 2017, 3:40 p.m. UTC
  These __need macros are only used internally, by nptl/descr.h.
However, including all of resolv.h from descr.h causes build failures
due to resolv.h's dozens of pseudo-struct-field macros, some of which
collide with struct fields in NPTL internal data structures.
Similarly, including all of list.h from descr.h produces an include
cycle, atomic.h -> atomic-machine.h -> tls.h -> descr.h -> list.h ->
atomic.h, and then list.h tries to use atomic.h macros that haven't
been defined yet.  So we do need mini-headers for these.  In the
list.h case I called it include/list_t.h since it isn't going to be
installed.

	* resolv/resolv.h: Remove __need_res_state logic.
	Move definition of res_state and related constants to ...
	* resolv/bits/types/res_state.h: ...this new file.
	* resolv/Makefile: Install bits/types/res_state.h.
	* include/bits/types/res_state.h: New wrapper.
	* include/list.h: Remove __need_list_t logic.
	Move definition of list_t to ...
	* include/list_t.h: ...this new file.

	* nptl/descr.h: Include list_t.h and bits/types/res_state.h
	instead of list.h and resolv.h.
---
 include/bits/types/res_state.h |  1 +
 include/list.h                 | 38 +++++-------------
 include/list_t.h               | 31 +++++++++++++++
 nptl/descr.h                   |  6 +--
 resolv/Makefile                |  2 +-
 resolv/bits/types/res_state.h  | 71 +++++++++++++++++++++++++++++++++
 resolv/resolv.h                | 90 ++++--------------------------------------
 7 files changed, 123 insertions(+), 116 deletions(-)
 create mode 100644 include/bits/types/res_state.h
 create mode 100644 include/list_t.h
 create mode 100644 resolv/bits/types/res_state.h
  

Comments

Florian Weimer May 10, 2017, 7:37 a.m. UTC | #1
On 05/09/2017 05:40 PM, Zack Weinberg wrote:
> +#define LOCALDOMAINPARTS	2	/* min levels in name that is "local" */
> +
> +#define RES_TIMEOUT		5	/* min. seconds between retries */
> +#define MAXRESOLVSORT		10	/* number of net to sort on */
> +#define RES_MAXNDOTS		15	/* should reflect bit field size */
> +#define RES_MAXRETRANS		30	/* only for resolv.conf/RES_OPTIONS */
> +#define RES_MAXRETRY		5	/* only for resolv.conf/RES_OPTIONS */
> +#define RES_DFLRETRY		2	/* Default #/tries. */
> +#define RES_MAXTIME		65535	/* Infinity, in milliseconds. */
> +
> +struct __res_state {
> +	int	retrans;		/* retransmition time interval */
> +	int	retry;			/* number of times to retransmit */
> +	unsigned long options;		/* option flags - see below. */
> +	int	nscount;		/* number of name servers */
> +	struct sockaddr_in
> +		nsaddr_list[MAXNS];	/* address of name server */
> +# define nsaddr	nsaddr_list[0]		/* for backward compatibility */

I suggest to keep all those #defines in <resolv/resolv.h>, purely to 
minimize recompilation if we add more macros here.  This applies to 
RES_*, LOCALDOMAINPARTS and the nsaddr macro.  MAXNS, MAXDFLSRCH, 
MAXDNSRCH can stay because we can't change them anyway.

Rest of the patch looks okay, but please verify that this does not 
affect stripped binaries.

Thanks,
Florian
  
Zack Weinberg May 20, 2017, 7:37 p.m. UTC | #2
On 05/10/2017 03:37 AM, Florian Weimer wrote:
> On 05/09/2017 05:40 PM, Zack Weinberg wrote:
>> +#define LOCALDOMAINPARTS    2    /* min levels in name that is
>> "local" */
>> +
>> +#define RES_TIMEOUT        5    /* min. seconds between retries */
>> +#define MAXRESOLVSORT        10    /* number of net to sort on */
>> +#define RES_MAXNDOTS        15    /* should reflect bit field size */
>> +#define RES_MAXRETRANS        30    /* only for
>> resolv.conf/RES_OPTIONS */
>> +#define RES_MAXRETRY        5    /* only for resolv.conf/RES_OPTIONS */
>> +#define RES_DFLRETRY        2    /* Default #/tries. */
>> +#define RES_MAXTIME        65535    /* Infinity, in milliseconds. */
>> +
>> +struct __res_state {
>> +    int    retrans;        /* retransmition time interval */
>> +    int    retry;            /* number of times to retransmit */
>> +    unsigned long options;        /* option flags - see below. */
>> +    int    nscount;        /* number of name servers */
>> +    struct sockaddr_in
>> +        nsaddr_list[MAXNS];    /* address of name server */
>> +# define nsaddr    nsaddr_list[0]        /* for backward
>> compatibility */
> 
> I suggest to keep all those #defines in <resolv/resolv.h>, purely to
> minimize recompilation if we add more macros here.  This applies to
> RES_*, LOCALDOMAINPARTS and the nsaddr macro.  MAXNS, MAXDFLSRCH,
> MAXDNSRCH can stay because we can't change them anyway.

Ok.

> Rest of the patch looks okay, but please verify that this does not
> affect stripped binaries.

Indeed, it does not.
  

Patch

diff --git a/include/bits/types/res_state.h b/include/bits/types/res_state.h
new file mode 100644
index 0000000000..4b99b68da9
--- /dev/null
+++ b/include/bits/types/res_state.h
@@ -0,0 +1 @@ 
+#include <resolv/bits/types/res_state.h>
diff --git a/include/list.h b/include/list.h
index 6846783594..dec5deef8e 100644
--- a/include/list.h
+++ b/include/list.h
@@ -17,39 +17,23 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #ifndef _LIST_H
+#define _LIST_H	1
 
-#ifndef __need_list_t
-# define _LIST_H	1
-#endif
+/* Internal: doubly linked lists.  */
 
 /* The definitions of this file are adopted from those which can be
    found in the Linux kernel headers to enable people familiar with
    the latter find their way in these sources as well.  */
 
-
-#if defined __need_list_t || defined _LIST_H
-# ifndef __list_t_defined
-#  define __list_t_defined
-/* Basic type for the double-link list.  */
-typedef struct list_head
-{
-  struct list_head *next;
-  struct list_head *prev;
-} list_t;
-# endif
-# undef __need_list_t
-#endif
-
-#ifdef _LIST_H
-
-# include <atomic.h>
+#include <list_t.h>
+#include <atomic.h>
 
 /* Define a variable with the head and tail of the list.  */
-# define LIST_HEAD(name) \
+#define LIST_HEAD(name) \
   list_t name = { &(name), &(name) }
 
 /* Initialize a new list head.  */
-# define INIT_LIST_HEAD(ptr) \
+#define INIT_LIST_HEAD(ptr) \
   (ptr)->next = (ptr)->prev = (ptr)
 
 
@@ -90,28 +74,26 @@  list_splice (list_t *add, list_t *head)
 
 
 /* Get typed element from list at a given position.  */
-# define list_entry(ptr, type, member) \
+#define list_entry(ptr, type, member) \
   ((type *) ((char *) (ptr) - (unsigned long) (&((type *) 0)->member)))
 
 
 
 /* Iterate forward over the elements of the list.  */
-# define list_for_each(pos, head) \
+#define list_for_each(pos, head) \
   for (pos = (head)->next; pos != (head); pos = pos->next)
 
 
 /* Iterate forward over the elements of the list.  */
-# define list_for_each_prev(pos, head) \
+#define list_for_each_prev(pos, head) \
   for (pos = (head)->prev; pos != (head); pos = pos->prev)
 
 
 /* Iterate backwards over the elements list.  The list elements can be
    removed from the list while doing this.  */
-# define list_for_each_prev_safe(pos, p, head) \
+#define list_for_each_prev_safe(pos, p, head) \
   for (pos = (head)->prev, p = pos->prev; \
        pos != (head); \
        pos = p, p = pos->prev)
 
-#endif /* _LIST_H */
-
 #endif	/* list.h */
diff --git a/include/list_t.h b/include/list_t.h
new file mode 100644
index 0000000000..d737e2e214
--- /dev/null
+++ b/include/list_t.h
@@ -0,0 +1,31 @@ 
+/* Copyright (C) 2002-2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _LIST_T_H
+#define _LIST_T_H 1
+
+/* Internal: doubly linked lists.  */
+
+/* Basic type for the double-link list.  */
+typedef struct list_head
+{
+  struct list_head *next;
+  struct list_head *prev;
+} list_t;
+
+#endif /* list_t.h */
diff --git a/nptl/descr.h b/nptl/descr.h
index a145860f07..c5ad0c8dba 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -25,16 +25,14 @@ 
 #include <stdbool.h>
 #include <sys/types.h>
 #include <hp-timing.h>
-#define __need_list_t
-#include <list.h>
+#include <list_t.h>
 #include <lowlevellock.h>
 #include <pthreaddef.h>
 #include <dl-sysdep.h>
 #include "../nptl_db/thread_db.h"
 #include <tls.h>
 #include <unwind.h>
-#define __need_res_state
-#include <resolv.h>
+#include <bits/types/res_state.h>
 #include <kernel-features.h>
 
 #ifndef TCB_ALIGNMENT
diff --git a/resolv/Makefile b/resolv/Makefile
index d41fd4603d..70b6f4ffe8 100644
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -22,7 +22,7 @@  subdir	:= resolv
 
 include ../Makeconfig
 
-headers	:= resolv.h \
+headers	:= resolv.h bits/types/res_state.h \
 	   netdb.h bits/netdb.h \
 	   arpa/nameser.h arpa/nameser_compat.h \
 	   sys/bitypes.h
diff --git a/resolv/bits/types/res_state.h b/resolv/bits/types/res_state.h
new file mode 100644
index 0000000000..f4cca385ae
--- /dev/null
+++ b/resolv/bits/types/res_state.h
@@ -0,0 +1,71 @@ 
+#ifndef __res_state_defined
+#define __res_state_defined 1
+
+#include <sys/types.h>
+#include <netinet/in.h>
+
+/* res_state: the global state used by the resolver stub.  */
+
+#define MAXNS			3	/* max # name servers we'll track */
+#define MAXDFLSRCH		3	/* # default domain levels to try */
+#define MAXDNSRCH		6	/* max # domains in search path */
+#define LOCALDOMAINPARTS	2	/* min levels in name that is "local" */
+
+#define RES_TIMEOUT		5	/* min. seconds between retries */
+#define MAXRESOLVSORT		10	/* number of net to sort on */
+#define RES_MAXNDOTS		15	/* should reflect bit field size */
+#define RES_MAXRETRANS		30	/* only for resolv.conf/RES_OPTIONS */
+#define RES_MAXRETRY		5	/* only for resolv.conf/RES_OPTIONS */
+#define RES_DFLRETRY		2	/* Default #/tries. */
+#define RES_MAXTIME		65535	/* Infinity, in milliseconds. */
+
+struct __res_state {
+	int	retrans;		/* retransmition time interval */
+	int	retry;			/* number of times to retransmit */
+	unsigned long options;		/* option flags - see below. */
+	int	nscount;		/* number of name servers */
+	struct sockaddr_in
+		nsaddr_list[MAXNS];	/* address of name server */
+# define nsaddr	nsaddr_list[0]		/* for backward compatibility */
+	unsigned short id;		/* current message id */
+	/* 2 byte hole here.  */
+	char	*dnsrch[MAXDNSRCH+1];	/* components of domain to search */
+	char	defdname[256];		/* default domain (deprecated) */
+	unsigned long pfcode;		/* RES_PRF_ flags - see below. */
+	unsigned ndots:4;		/* threshold for initial abs. query */
+	unsigned nsort:4;		/* number of elements in sort_list[] */
+	unsigned ipv6_unavail:1;	/* connecting to IPv6 server failed */
+	unsigned unused:23;
+	struct {
+		struct in_addr	addr;
+		uint32_t	mask;
+	} sort_list[MAXRESOLVSORT];
+	/* 4 byte hole here on 64-bit architectures.  */
+	void * __glibc_unused_qhook;
+	void * __glibc_unused_rhook;
+	int	res_h_errno;		/* last one set for this context */
+	int	_vcsock;		/* PRIVATE: for res_send VC i/o */
+	unsigned int _flags;		/* PRIVATE: see below */
+	/* 4 byte hole here on 64-bit architectures.  */
+	union {
+		char	pad[52];	/* On an i386 this means 512b total. */
+		struct {
+			uint16_t		nscount;
+			uint16_t		nsmap[MAXNS];
+			int			nssocks[MAXNS];
+			uint16_t		nscount6;
+			uint16_t		nsinit;
+			struct sockaddr_in6	*nsaddrs[MAXNS];
+#ifdef _LIBC
+			unsigned long long int	initstamp
+			  __attribute__((packed));
+#else
+			unsigned int		_initstamp[2];
+#endif
+		} _ext;
+	} _u;
+};
+
+typedef struct __res_state *res_state;
+
+#endif /* __res_state_defined */
diff --git a/resolv/resolv.h b/resolv/resolv.h
index 9fef8e9f24..c95cf3309a 100644
--- a/resolv/resolv.h
+++ b/resolv/resolv.h
@@ -50,92 +50,16 @@ 
  */
 
 #ifndef _RESOLV_H_
+#define _RESOLV_H_
 
-/* These headers are needed for types used in the `struct res_state'
-   declaration.  */
+#include <sys/cdefs.h>
+#include <sys/param.h>
 #include <sys/types.h>
+#include <stdio.h>
 #include <netinet/in.h>
+#include <arpa/nameser.h>
+#include <bits/types/res_state.h>
 
-#ifndef __need_res_state
-# define _RESOLV_H_
-
-# include <sys/param.h>
-# include <sys/cdefs.h>
-# include <stdio.h>
-# include <arpa/nameser.h>
-#endif
-
-#ifndef __res_state_defined
-# define __res_state_defined
-
-/*
- * Global defines and variables for resolver stub.
- */
-# define MAXNS			3	/* max # name servers we'll track */
-# define MAXDFLSRCH		3	/* # default domain levels to try */
-# define MAXDNSRCH		6	/* max # domains in search path */
-# define LOCALDOMAINPARTS	2	/* min levels in name that is "local" */
-
-# define RES_TIMEOUT		5	/* min. seconds between retries */
-# define MAXRESOLVSORT		10	/* number of net to sort on */
-# define RES_MAXNDOTS		15	/* should reflect bit field size */
-# define RES_MAXRETRANS		30	/* only for resolv.conf/RES_OPTIONS */
-# define RES_MAXRETRY		5	/* only for resolv.conf/RES_OPTIONS */
-# define RES_DFLRETRY		2	/* Default #/tries. */
-# define RES_MAXTIME		65535	/* Infinity, in milliseconds. */
-
-struct __res_state {
-	int	retrans;		/* retransmition time interval */
-	int	retry;			/* number of times to retransmit */
-	unsigned long options;		/* option flags - see below. */
-	int	nscount;		/* number of name servers */
-	struct sockaddr_in
-		nsaddr_list[MAXNS];	/* address of name server */
-# define nsaddr	nsaddr_list[0]		/* for backward compatibility */
-	unsigned short id;		/* current message id */
-	/* 2 byte hole here.  */
-	char	*dnsrch[MAXDNSRCH+1];	/* components of domain to search */
-	char	defdname[256];		/* default domain (deprecated) */
-	unsigned long pfcode;		/* RES_PRF_ flags - see below. */
-	unsigned ndots:4;		/* threshold for initial abs. query */
-	unsigned nsort:4;		/* number of elements in sort_list[] */
-	unsigned ipv6_unavail:1;	/* connecting to IPv6 server failed */
-	unsigned unused:23;
-	struct {
-		struct in_addr	addr;
-		uint32_t	mask;
-	} sort_list[MAXRESOLVSORT];
-	/* 4 byte hole here on 64-bit architectures.  */
-	void * __glibc_unused_qhook;
-	void * __glibc_unused_rhook;
-	int	res_h_errno;		/* last one set for this context */
-	int	_vcsock;		/* PRIVATE: for res_send VC i/o */
-	unsigned int _flags;		/* PRIVATE: see below */
-	/* 4 byte hole here on 64-bit architectures.  */
-	union {
-		char	pad[52];	/* On an i386 this means 512b total. */
-		struct {
-			uint16_t		nscount;
-			uint16_t		nsmap[MAXNS];
-			int			nssocks[MAXNS];
-			uint16_t		nscount6;
-			uint16_t		nsinit;
-			struct sockaddr_in6	*nsaddrs[MAXNS];
-#ifdef _LIBC
-			unsigned long long int	initstamp
-			  __attribute__((packed));
-#else
-			unsigned int		_initstamp[2];
-#endif
-		} _ext;
-	} _u;
-};
-
-typedef struct __res_state *res_state;
-# undef __need_res_state
-#endif
-
-#ifdef _RESOLV_H_
 /*
  * Revision information.  This is the release date in YYYYMMDD format.
  * It can change every day so the right thing to do with it is use it
@@ -361,7 +285,7 @@  int		res_nmkquery (res_state, int, const char *, int, int,
 int		res_nsend (res_state, const unsigned char *, int,
 			   unsigned char *, int) __THROW;
 void		res_nclose (res_state) __THROW;
+
 __END_DECLS
-#endif
 
 #endif /* !_RESOLV_H_ */