[PATCH 00/11] Porting nfs-utils to musl libc

[PATCH 00/11] Porting nfs-utils to musl libc

[Natanael Copa]

Musl libc is a modern libc for Linux which focuses on correctness in
standards-conformance. We use this libc for Alpine Linux.

This patchset makes nfs-utils build with musl libc and should generally
improve portability.

It would be nice if at least some of the patches could be applied
upstream so we can reduce the number of patches for getting nfs working
on Alpine Linux.

Natanael Copa (11):
  conffile: use standard uint*_t and unsigned char
  Fix header include for definition of NULL
  replace __attribute_malloc__ with the more portable
    __attribute__((__malloc__))
  exportfs: define _GNU_SOURCE for stat64
  mountd: define _GNU_SOURCE for statfs64 and use standard dev_t
  nfsstat: replace the legacy SA_ONESHOT with standard SA_RESETHAND
  Allow usage of getrpcbynumber() when getrpcbynumber_r() is unavailable
  include libgen.h for basename
  Only work around glibc bugs on glibc
  exportfs: fix test of NULL pointer in host_pton()
  exportfs: only do glibc specific hackery on glibc

 configure.ac                 |  6 +-----
 support/export/hostname.c    | 29 +++++++++++++++++------------
 support/include/conffile.h   |  2 +-
 support/include/exportfs.h   | 10 +++++-----
 support/include/sockaddr.h   |  1 +
 support/nfs/conffile.c       | 14 +++++++-------
 support/nfs/svc_create.c     |  2 +-
 support/nfs/svc_socket.c     |  6 ++++++
 tools/rpcdebug/rpcdebug.c    |  1 +
 utils/exportfs/exportfs.c    |  4 ++++
 utils/mount/mount.c          |  1 +
 utils/mount/mount_libmount.c |  1 +
 utils/mountd/cache.c         |  6 +++++-
 utils/mountd/svc_run.c       |  2 +-
 utils/nfsd/nfsd.c            |  1 +
 utils/nfsidmap/nfsidmap.c    |  1 +
 utils/nfsstat/nfsstat.c      |  2 +-
 utils/statd/hostname.c       |  6 +++---
 utils/statd/sm-notify.c      |  8 ++++----
 utils/statd/statd.h          |  2 +-
 20 files changed, 63 insertions(+), 42 deletions(-)

-- 
2.0.3

[PATCH 01/11] conffile: use standard uint*_t and unsigned char

[Natanael Copa]

Use the standard integer types. This fixes compiling errors with musl
libc.

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 support/include/conffile.h |  2 +-
 support/nfs/conffile.c     | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index 05ea5d2..94fb005 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -49,7 +49,7 @@ struct conf_list {
 extern char    *conf_path;
 
 extern int      conf_begin(void);
-extern int      conf_decode_base64(u_int8_t *, u_int32_t *, u_char *);
+extern int      conf_decode_base64(uint8_t *, uint32_t *, unsigned char *);
 extern int      conf_end(int, int);
 extern void     conf_free_list(struct conf_list *);
 extern struct sockaddr *conf_get_address(char *, char *);
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index c3434d5..6b94ec0 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -72,10 +72,10 @@ TAILQ_HEAD (conf_trans_head, conf_trans) conf_trans_queue;
 /*
  * Radix-64 Encoding.
  */
-static const u_int8_t bin2asc[]
+static const uint8_t bin2asc[]
   = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
 
-static const u_int8_t asc2bin[] =
+static const uint8_t asc2bin[] =
 {
   255, 255, 255, 255, 255, 255, 255, 255,
   255, 255, 255, 255, 255, 255, 255, 255,
@@ -109,10 +109,10 @@ LIST_HEAD (conf_bindings, conf_binding) conf_bindings[256];
 
 static char *conf_addr;
 
-static __inline__ u_int8_t
+static __inline__ uint8_t
 conf_hash(char *s)
 {
-	u_int8_t hash = 0;
+	uint8_t hash = 0;
 
 	while (*s) {
 		hash = ((hash << 1) | (hash >> 7)) ^ tolower (*s);
@@ -603,10 +603,10 @@ cleanup:
 
 /* Decode a PEM encoded buffer.  */
 int
-conf_decode_base64 (u_int8_t *out, u_int32_t *len, u_char *buf)
+conf_decode_base64 (uint8_t *out, uint32_t *len, unsigned char *buf)
 {
-	u_int32_t c = 0;
-	u_int8_t c1, c2, c3, c4;
+	uint32_t c = 0;
+	uint8_t c1, c2, c3, c4;
 
 	while (*buf) {
 		if (*buf > 127 || (c1 = asc2bin[*buf]) == 255)
-- 
2.0.3

[PATCH 02/11] Fix header include for definition of NULL

[Natanael Copa]

NULL is defined in stdlib.h so we need to include that.

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 support/include/sockaddr.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/support/include/sockaddr.h b/support/include/sockaddr.h
index a1c30f9..446b537 100644
--- a/support/include/sockaddr.h
+++ b/support/include/sockaddr.h
@@ -27,6 +27,7 @@
 #ifdef HAVE_LIBIO_H
 #include <libio.h>
 #endif
+#include <stdlib.h>
 #include <stdbool.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
-- 
2.0.3

[PATCH 03/11] replace __attribute_malloc__ with the more portable __attribute__((__malloc__))

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 support/export/hostname.c  | 14 +++++++-------
 support/include/exportfs.h | 10 +++++-----
 support/nfs/svc_create.c   |  2 +-
 utils/statd/hostname.c     |  6 +++---
 utils/statd/sm-notify.c    |  8 ++++----
 utils/statd/statd.h        |  2 +-
 6 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/support/export/hostname.c b/support/export/hostname.c
index 3e949a1..ad595d1 100644
--- a/support/export/hostname.c
+++ b/support/export/hostname.c
@@ -91,7 +91,7 @@ host_ntop(const struct sockaddr *sap, char *buf, const size_t buflen)
  * Returns address info structure, or NULL if an error occurs.  Caller
  * must free the returned structure with freeaddrinfo(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *
 host_pton(const char *paddr)
 {
@@ -153,7 +153,7 @@ host_pton(const char *paddr)
  * if no information is available for @hostname.  Caller must free the
  * returned structure with freeaddrinfo(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *
 host_addrinfo(const char *hostname)
 {
@@ -199,7 +199,7 @@ host_addrinfo(const char *hostname)
  * the string.
  */
 #ifdef HAVE_GETNAMEINFO
-__attribute_malloc__
+__attribute__((__malloc__))
 char *
 host_canonname(const struct sockaddr *sap)
 {
@@ -234,7 +234,7 @@ host_canonname(const struct sockaddr *sap)
 	return strdup(buf);
 }
 #else	/* !HAVE_GETNAMEINFO */
-__attribute_malloc__
+__attribute__((__malloc__))
 char *
 host_canonname(const struct sockaddr *sap)
 {
@@ -266,7 +266,7 @@ host_canonname(const struct sockaddr *sap)
  *
  * Caller must free the returned structure with freeaddrinfo(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *
 host_reliable_addrinfo(const struct sockaddr *sap)
 {
@@ -313,7 +313,7 @@ out_free_hostname:
  * Caller must free the returned structure with freeaddrinfo(3).
  */
 #ifdef HAVE_GETNAMEINFO
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *
 host_numeric_addrinfo(const struct sockaddr *sap)
 {
@@ -361,7 +361,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
 	return ai;
 }
 #else	/* !HAVE_GETNAMEINFO */
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *
 host_numeric_addrinfo(const struct sockaddr *sap)
 {
diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index 97b2327..9021fae 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -156,15 +156,15 @@ int				secinfo_addflavor(struct flav_info *, struct exportent *);
 
 char *				host_ntop(const struct sockaddr *sap,
 						char *buf, const size_t buflen);
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *		host_pton(const char *paddr);
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *		host_addrinfo(const char *hostname);
-__attribute_malloc__
+__attribute__((__malloc__))
 char *				host_canonname(const struct sockaddr *sap);
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *		host_reliable_addrinfo(const struct sockaddr *sap);
-__attribute_malloc__
+__attribute__((__malloc__))
 struct addrinfo *		host_numeric_addrinfo(const struct sockaddr *sap);
 
 int				rmtab_read(void);
diff --git a/support/nfs/svc_create.c b/support/nfs/svc_create.c
index 6b9e85b..a706f87 100644
--- a/support/nfs/svc_create.c
+++ b/support/nfs/svc_create.c
@@ -113,7 +113,7 @@ svc_create_find_xprt(const struct sockaddr *bindaddr, const struct netconfig *nc
  *
  * Otherwise NULL is returned if an error occurs.
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct addrinfo *
 svc_create_bindaddr(struct netconfig *nconf, const uint16_t port)
 {
diff --git a/utils/statd/hostname.c b/utils/statd/hostname.c
index 746ecc7..c61087c 100644
--- a/utils/statd/hostname.c
+++ b/utils/statd/hostname.c
@@ -105,7 +105,7 @@ statd_present_address(const struct sockaddr *sap, char *buf, const size_t buflen
  * Look up the hostname; report exceptional errors.  Caller must
  * call freeaddrinfo(3) if a valid addrinfo is returned.
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct addrinfo *
 get_addrinfo(const char *hostname, const struct addrinfo *hint)
 {
@@ -184,7 +184,7 @@ get_nameinfo(const struct sockaddr *sap,
  * We won't monitor peers that don't have a reverse map.  The canonical
  * name gives us a key for our monitor list.
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 char *
 statd_canonical_name(const char *hostname)
 {
@@ -234,7 +234,7 @@ statd_canonical_name(const char *hostname)
  * NULL if some error occurs.  Caller must free the returned
  * list with freeaddrinfo(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct addrinfo *
 statd_canonical_list(const char *hostname)
 {
diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 9dbe5d9..5994b2f 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -74,7 +74,7 @@ static int		record_pid(void);
 
 static struct nsm_host *	hosts = NULL;
 
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct addrinfo *
 smn_lookup(const char *name)
 {
@@ -149,7 +149,7 @@ smn_get_hostname(const struct sockaddr *sap,
  * if the canonical name doesn't exist or cannot be determined.
  * The caller must free the result with free(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static char *
 smn_verify_my_name(const char *name)
 {
@@ -189,7 +189,7 @@ smn_verify_my_name(const char *name)
 	return retval;
 }
 
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct nsm_host *
 smn_alloc_host(const char *hostname, const char *mon_name,
 		const char *my_name, const time_t timestamp)
@@ -343,7 +343,7 @@ static int smn_socket(void)
  * If admin specified a source address or srcport, then convert those
  * to a sockaddr and return it.   Otherwise, return an ANYADDR address.
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static struct addrinfo *
 smn_bind_address(const char *srcaddr, const char *srcport)
 {
diff --git a/utils/statd/statd.h b/utils/statd/statd.h
index e89e666..a1d8035 100644
--- a/utils/statd/statd.h
+++ b/utils/statd/statd.h
@@ -25,7 +25,7 @@
 extern _Bool	statd_matchhostname(const char *hostname1, const char *hostname2);
 extern _Bool	statd_present_address(const struct sockaddr *sap, char *buf,
 					const size_t buflen);
-__attribute_malloc__
+__attribute__((__malloc__))
 extern char *	statd_canonical_name(const char *hostname);
 
 extern void	my_svc_run(void);
-- 
2.0.3

[PATCH 04/11] exportfs: define _GNU_SOURCE for stat64

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 utils/exportfs/exportfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index bf07555..7ab93d1 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -12,6 +12,10 @@
 #include <config.h>
 #endif
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/vfs.h>
-- 
2.0.3

[PATCH 05/11] mountd: define _GNU_SOURCE for statfs64 and use standard dev_t

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 utils/mountd/cache.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index b0cc6a8..b36255b 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -11,6 +11,10 @@
 #include <config.h>
 #endif
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <sys/types.h>
 #include <sys/select.h>
 #include <sys/stat.h>
@@ -1419,7 +1423,7 @@ static int cache_export_ent(char *domain, struct exportent *exp, char *path)
 		 */
 		struct stat stb;
 		size_t l = strlen(exp->e_path);
-		__dev_t dev;
+		dev_t dev;
 
 		if (strlen(path) <= l || path[l] != '/' ||
 		    strncmp(exp->e_path, path, l) != 0)
-- 
2.0.3

[PATCH 06/11] nfsstat: replace the legacy SA_ONESHOT with standard SA_RESETHAND

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 utils/nfsstat/nfsstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/nfsstat/nfsstat.c b/utils/nfsstat/nfsstat.c
index 70f8d10..3612bfe 100644
--- a/utils/nfsstat/nfsstat.c
+++ b/utils/nfsstat/nfsstat.c
@@ -336,7 +336,7 @@ main(int argc, char **argv)
 
 	struct sigaction act = {
 		.sa_handler = unpause,
-		.sa_flags = SA_ONESHOT,
+		.sa_flags = SA_RESETHAND,
 	};
 
 	if ((progname = strrchr(argv[0], '/')))
-- 
2.0.3

[PATCH 07/11] Allow usage of getrpcbynumber() when getrpcbynumber_r() is unavailable

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 configure.ac             | 6 +-----
 support/nfs/svc_socket.c | 6 ++++++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 408f4c8..42cd3e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -247,9 +247,6 @@ AC_CHECK_FUNC([connect], ,
 AC_CHECK_FUNC([getaddrinfo], ,
               [AC_MSG_ERROR([Function 'getaddrinfo' not found.])])
 
-AC_CHECK_FUNC([getrpcbynumber], ,
-              [AC_MSG_ERROR([Function 'getrpcbynumber' not found.])])
-
 AC_CHECK_FUNC([getservbyname], ,
               [AC_MSG_ERROR([Function 'getservbyname' not found.])])
 
@@ -408,12 +405,11 @@ AC_FUNC_STAT
 AC_FUNC_VPRINTF
 AC_CHECK_FUNCS([alarm atexit dup2 fdatasync ftruncate getcwd \
                gethostbyaddr gethostbyname gethostname getmntent \
-               getnameinfo getrpcbyname getifaddrs \
+               getnameinfo getrpcbyname getrpcbynumber getrpcbynumber_r getifaddrs \
                gettimeofday hasmntopt inet_ntoa innetgr memset mkdir pathconf \
                ppoll realpath rmdir select socket strcasecmp strchr strdup \
                strerror strrchr strtol strtoul sigprocmask name_to_handle_at])
 
-
 dnl *************************************************************
 dnl Check for data sizes
 dnl *************************************************************
diff --git a/support/nfs/svc_socket.c b/support/nfs/svc_socket.c
index f56f310..61ccf5b 100644
--- a/support/nfs/svc_socket.c
+++ b/support/nfs/svc_socket.c
@@ -42,8 +42,14 @@ int getservport(u_long number, const char *proto)
 	struct servent servbuf, *servp = NULL;
 	int ret;
 
+#if HAVE_GETRPCBYNUMBER_R
 	ret = getrpcbynumber_r(number, &rpcbuf, rpcdata, sizeof rpcdata,
 				&rpcp);
+#else
+	rpcp = getrpcbynumber(number);
+	ret = 0;
+#endif
+
 	if (ret == 0 && rpcp != NULL) {
 		/* First try name.  */
 		ret = getservbyname_r(rpcp->r_name, proto, &servbuf, servdata,
-- 
2.0.3

[PATCH 08/11] include libgen.h for basename

[Natanael Copa]

According POSIX basename(3) should have an #include <libgen.h>

There are a different GNU implementation too, that can be used with
_GNU_SOURCE, but the POSIX version is good enough and more portable.

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 tools/rpcdebug/rpcdebug.c    | 1 +
 utils/mount/mount.c          | 1 +
 utils/mount/mount_libmount.c | 1 +
 utils/nfsd/nfsd.c            | 1 +
 utils/nfsidmap/nfsidmap.c    | 1 +
 5 files changed, 5 insertions(+)

diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
index d6e10d3..18b1622 100644
--- a/tools/rpcdebug/rpcdebug.c
+++ b/tools/rpcdebug/rpcdebug.c
@@ -26,6 +26,7 @@
 #include <malloc.h>
 #include <fcntl.h>
 #include <ctype.h>
+#include <libgen.h>
 /* RPC debug flags
    #include <sunrpc/debug.h> */
 /* NFS debug flags
diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index eea00af..91f1087 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -33,6 +33,7 @@
 #include <getopt.h>
 #include <mntent.h>
 #include <pwd.h>
+#include <libgen.h>
 
 #include "fstab.h"
 #include "xcommon.h"
diff --git a/utils/mount/mount_libmount.c b/utils/mount/mount_libmount.c
index 701d41e..6f85dc9 100644
--- a/utils/mount/mount_libmount.c
+++ b/utils/mount/mount_libmount.c
@@ -29,6 +29,7 @@
 #include <string.h>
 #include <errno.h>
 #include <getopt.h>
+#include <libgen.h>
 
 #include <libmount/libmount.h>
 
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 03e3c81..201bb13 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -19,6 +19,7 @@
 #include <errno.h>
 #include <getopt.h>
 #include <netdb.h>
+#include <libgen.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 3f51b4d..e0d31e7 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -4,6 +4,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
+#include <libgen.h>
 
 #include <pwd.h>
 #include <grp.h>
-- 
2.0.3

[PATCH 09/11] Only work around glibc bugs on glibc

[Natanael Copa]

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 utils/mountd/svc_run.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c
index 1938a67..a572441 100644
--- a/utils/mountd/svc_run.c
+++ b/utils/mountd/svc_run.c
@@ -60,7 +60,7 @@
 void cache_set_fds(fd_set *fdset);
 int cache_process_req(fd_set *readfds);
 
-#if LONG_MAX != INT_MAX	
+#if defined(__GLIBC__) && LONG_MAX != INT_MAX
 /* bug in glibc 2.3.6 and earlier, we need
  * our own svc_getreqset
  */
-- 
2.0.3

[PATCH 10/11] exportfs: fix test of NULL pointer in host_pton()

[Natanael Copa]

should fix https://bugzilla.redhat.com/show_bug.cgi?id=1083018

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 support/export/hostname.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/support/export/hostname.c b/support/export/hostname.c
index ad595d1..d9153e1 100644
--- a/support/export/hostname.c
+++ b/support/export/hostname.c
@@ -115,6 +115,11 @@ host_pton(const char *paddr)
 	 * have a real AF_INET presentation address, before invoking
 	 * getaddrinfo(3) to generate the full addrinfo list.
 	 */
+	if (paddr == NULL) {
+		xlog(D_GENERAL, "%s: passed a NULL presentation address",
+			__func__);
+		return NULL;
+	}
 	inet4 = 1;
 	if (inet_pton(AF_INET, paddr, &sin.sin_addr) == 0)
 		inet4 = 0;
@@ -123,15 +128,12 @@ host_pton(const char *paddr)
 	switch (error) {
 	case 0:
 		if (!inet4 && ai->ai_addr->sa_family == AF_INET) {
+			xlog(D_GENERAL, "%s: failed to convert %s",
+					__func__, paddr);
 			freeaddrinfo(ai);
 			break;
 		}
 		return ai;
-	case EAI_NONAME:
-		if (paddr == NULL)
-			xlog(D_GENERAL, "%s: passed a NULL presentation address",
-				__func__);
-		break;
 	case EAI_SYSTEM:
 		xlog(D_GENERAL, "%s: failed to convert %s: (%d) %m",
 				__func__, paddr, errno);
-- 
2.0.3

[PATCH 11/11] exportfs: only do glibc specific hackery on glibc

[Natanael Copa]

We should not depend on the libc do free(3) on ai_canonname as that is
completely up to implementation and known o break things on uclibc and
musl libc.

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 support/export/hostname.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/support/export/hostname.c b/support/export/hostname.c
index d9153e1..30584b4 100644
--- a/support/export/hostname.c
+++ b/support/export/hostname.c
@@ -382,6 +382,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
 
 	ai = host_pton(buf);
 
+#if !definded(__UCLIBC__) && defined(__GLIBC__)
 	/*
 	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
 	 */
@@ -392,7 +393,9 @@ host_numeric_addrinfo(const struct sockaddr *sap)
 			ai = NULL;
 		}
 	}
+#endif
 
 	return ai;
 }
+
 #endif	/* !HAVE_GETNAMEINFO */
-- 
2.0.3

Re: [PATCH 11/11] exportfs: only do glibc specific hackery on glibc

[Jeff Layton]

On Wed, 30 Jul 2014 13:23:19 +0200
Natanael Copa <natanael.copa@gmail.com> wrote:

> We should not depend on the libc do free(3) on ai_canonname as that is
> completely up to implementation and known o break things on uclibc and
> musl libc.
> 
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
>  support/export/hostname.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index d9153e1..30584b4 100644
> --- a/support/export/hostname.c
> +++ b/support/export/hostname.c
> @@ -382,6 +382,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  
>  	ai = host_pton(buf);
>  
> +#if !definded(__UCLIBC__) && defined(__GLIBC__)

As I mentioned on IRC, there's a typo above...

>  	/*
>  	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
>  	 */
> @@ -392,7 +393,9 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  			ai = NULL;
>  		}
>  	}
> +#endif
>  
>  	return ai;
>  }
> +
>  #endif	/* !HAVE_GETNAMEINFO */


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 04/11] exportfs: define _GNU_SOURCE for stat64

[NeilBrown]

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

On Wed, 30 Jul 2014 13:23:12 +0200 Natanael Copa <natanael.copa@gmail.com>
wrote:

> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
>  utils/exportfs/exportfs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index bf07555..7ab93d1 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -12,6 +12,10 @@
>  #include <config.h>
>  #endif
>  
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/vfs.h>

These all look really sensible!

One small suggestion: it would be really nice to see a comment in the code
explaining why _GNU_SOURCE is needed.  I suspect such comments get out of
date quickly, so maybe it wouldn't end up be all that useful.  But having a
comment is still, in my opinion, more useful than not.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 04/11] exportfs: define _GNU_SOURCE for stat64

[Jeff Layton]

On Thu, 31 Jul 2014 11:30:22 +1000
NeilBrown <neilb@suse.de> wrote:

> On Wed, 30 Jul 2014 13:23:12 +0200 Natanael Copa <natanael.copa@gmail.com>
> wrote:
> 
> > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> > ---
> >  utils/exportfs/exportfs.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> > index bf07555..7ab93d1 100644
> > --- a/utils/exportfs/exportfs.c
> > +++ b/utils/exportfs/exportfs.c
> > @@ -12,6 +12,10 @@
> >  #include <config.h>
> >  #endif
> >  
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE
> > +#endif
> > +
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/vfs.h>
> 
> These all look really sensible!
> 
> One small suggestion: it would be really nice to see a comment in the code
> explaining why _GNU_SOURCE is needed.  I suspect such comments get out of
> date quickly, so maybe it wouldn't end up be all that useful.  But having a
> comment is still, in my opinion, more useful than not.
> 
> Thanks,
> NeilBrown
> 

Agreed, they all look pretty sensible. That said, I think the supposed
best way to enable _GNU_SOURCE on autoconf projects is to use
AC_USE_SYSTEM_EXTENSIONS:

    https://www.gnu.org/software/autoconf/manual/autoconf-2.64/html_node/Posix-Variants.html

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 04/11] exportfs: define _GNU_SOURCE for stat64

[Steve Dickson]

On 07/31/2014 07:34 AM, Jeff Layton wrote:
> On Thu, 31 Jul 2014 11:30:22 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
>> On Wed, 30 Jul 2014 13:23:12 +0200 Natanael Copa <natanael.copa@gmail.com>
>> wrote:
>>
>>> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
>>> ---
>>>  utils/exportfs/exportfs.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>>> index bf07555..7ab93d1 100644
>>> --- a/utils/exportfs/exportfs.c
>>> +++ b/utils/exportfs/exportfs.c
>>> @@ -12,6 +12,10 @@
>>>  #include <config.h>
>>>  #endif
>>>  
>>> +#ifndef _GNU_SOURCE
>>> +#define _GNU_SOURCE
>>> +#endif
>>> +
>>>  #include <sys/types.h>
>>>  #include <sys/stat.h>
>>>  #include <sys/vfs.h>
>>
>> These all look really sensible!
>>
>> One small suggestion: it would be really nice to see a comment in the code
>> explaining why _GNU_SOURCE is needed.  I suspect such comments get out of
>> date quickly, so maybe it wouldn't end up be all that useful.  But having a
>> comment is still, in my opinion, more useful than not.
>>
>> Thanks,
>> NeilBrown
>>
> 
> Agreed, they all look pretty sensible. That said, I think the supposed
> best way to enable _GNU_SOURCE on autoconf projects is to use
> AC_USE_SYSTEM_EXTENSIONS:
> 
>     https://www.gnu.org/software/autoconf/manual/autoconf-2.64/html_node/Posix-Variants.html
> 
Its sounds like these requests are mutually exclusive... Meaning
if we use the AC_USE_SYSTEM_EXTENSIONS we will not get the
comments as to why _GNU_SOURCE is needed because the these 
#ifndefs and #define will go away... which I guess is OK...

Natanael, would you mind incorporate AC_USE_SYSTEM_EXTENSIONS as well
as fix the typo Jeff pointed out in patch 11? 

Once that happens... I'm good to go on these... 

steved.

Re: [PATCH 04/11] exportfs: define _GNU_SOURCE for stat64

[Natanael Copa]

On Tue, 05 Aug 2014 11:08:54 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 07/31/2014 07:34 AM, Jeff Layton wrote:
> > On Thu, 31 Jul 2014 11:30:22 +1000
> > NeilBrown <neilb@suse.de> wrote:
> > 
> >> On Wed, 30 Jul 2014 13:23:12 +0200 Natanael Copa <natanael.copa@gmail.com>
> >> wrote:
> >>
> >>> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> >>> ---
> >>>  utils/exportfs/exportfs.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> >>> index bf07555..7ab93d1 100644
> >>> --- a/utils/exportfs/exportfs.c
> >>> +++ b/utils/exportfs/exportfs.c
> >>> @@ -12,6 +12,10 @@
> >>>  #include <config.h>
> >>>  #endif
> >>>  
> >>> +#ifndef _GNU_SOURCE
> >>> +#define _GNU_SOURCE
> >>> +#endif
> >>> +
> >>>  #include <sys/types.h>
> >>>  #include <sys/stat.h>
> >>>  #include <sys/vfs.h>
> >>
> >> These all look really sensible!
> >>
> >> One small suggestion: it would be really nice to see a comment in the code
> >> explaining why _GNU_SOURCE is needed.  I suspect such comments get out of
> >> date quickly, so maybe it wouldn't end up be all that useful.  But having a
> >> comment is still, in my opinion, more useful than not.
> >>
> >> Thanks,
> >> NeilBrown
> >>
> > 
> > Agreed, they all look pretty sensible. That said, I think the supposed
> > best way to enable _GNU_SOURCE on autoconf projects is to use
> > AC_USE_SYSTEM_EXTENSIONS:
> > 
> >     https://www.gnu.org/software/autoconf/manual/autoconf-2.64/html_node/Posix-Variants.html
> > 
> Its sounds like these requests are mutually exclusive... Meaning
> if we use the AC_USE_SYSTEM_EXTENSIONS we will not get the
> comments as to why _GNU_SOURCE is needed because the these 
> #ifndefs and #define will go away... which I guess is OK...
> 
> Natanael, would you mind incorporate AC_USE_SYSTEM_EXTENSIONS as well
> as fix the typo Jeff pointed out in patch 11? 
> 
> Once that happens... I'm good to go on these... 

I sent a v2 set with the all the commented issues fixed to the list.

With AC_USE_SYSTEM_EXTENSIONS the patch with #include <libgen.h> should
not be needed in theory. But I kept it anyways as it should make things
more portable.

We are now working on another patch to make it actually run on musl...

Thanks for your feedback!


> 
> steved.

Re: [PATCH 11/11] exportfs: only do glibc specific hackery on glibc

[Steve Dickson]

On 07/30/2014 07:23 AM, Natanael Copa wrote:
> We should not depend on the libc do free(3) on ai_canonname as that is
> completely up to implementation and known o break things on uclibc and
> musl libc.
> 
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
>  support/export/hostname.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index d9153e1..30584b4 100644
> --- a/support/export/hostname.c
> +++ b/support/export/hostname.c
> @@ -382,6 +382,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  
>  	ai = host_pton(buf);
>  
> +#if !definded(__UCLIBC__) && defined(__GLIBC__)
You still have this typo here... and the only reason
it compiled is HAVE_GETNAMEINFO is not defined 
in your world....

How well were these change tested against glibc? I'm 
concern about eliminating chunks of need code with
all these new  defines.... 

steved.

>  	/*
>  	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
>  	 */
> @@ -392,7 +393,9 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  			ai = NULL;
>  		}
>  	}
> +#endif
>  
>  	return ai;
>  }
> +
>  #endif	/* !HAVE_GETNAMEINFO */
> 

Re: [PATCH 11/11] exportfs: only do glibc specific hackery on glibc

[Natanael Copa]

On Thu, 07 Aug 2014 08:15:01 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 07/30/2014 07:23 AM, Natanael Copa wrote:
> > We should not depend on the libc do free(3) on ai_canonname as that is
> > completely up to implementation and known o break things on uclibc and
> > musl libc.
> > 
> > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> > ---
> >  support/export/hostname.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/support/export/hostname.c b/support/export/hostname.c
> > index d9153e1..30584b4 100644
> > --- a/support/export/hostname.c
> > +++ b/support/export/hostname.c
> > @@ -382,6 +382,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
> >  
> >  	ai = host_pton(buf);
> >  
> > +#if !definded(__UCLIBC__) && defined(__GLIBC__)
> You still have this typo here... and the only reason
> it compiled is HAVE_GETNAMEINFO is not defined 
> in your world....

I do have HAVE_GETNAMEINFO or I wouldn't have this issue in first place.

$ grep HAVE_GETNAMEINFO support/include/config.h
#define HAVE_GETNAMEINFO 1


> How well were these change tested against glibc? I'm 
> concern about eliminating chunks of need code with
> all these new  defines.... 

Argh! Sorry my bad. I fixed but must missed it in a redo.

I have not tested those in glibc at all.

When I look now, this is wrong anyway. It was supposed to fix a
segfault due to a just-in-case free() below:

        /*
         * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
         */
        if (ai != NULL) {
                free(ai->ai_canonname);         /* just in case */
                ai->ai_canonname = strdup(buf);
                if (ai->ai_canonname == NULL) {
                        freeaddrinfo(ai);
                        ai = NULL;
                }
        }

I but I think my patch does the ifdef in wrong place. When I grep for
'never fills' in the sources i find that there are 2 places where there
is an

  ai->ai_canonname = strdup(buf);

With the assumption that freeaddrinfo() will free ai->ai_canonname for
us. I think this is wrong.

The just in case free(ai->ai_canonname) causes segfault on uclibc (and
used to do so on musl too) and I am pretty sure the "ai->ai_canonname =
strdup(buf);" will cause memleak, atleast on musl libc.

I'd be happy if you applied the other 10 patches though.

Do you want me to resend the others 10 with a v3 prefix?

-nc



> steved.
> 
> >  	/*
> >  	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
> >  	 */
> > @@ -392,7 +393,9 @@ host_numeric_addrinfo(const struct sockaddr *sap)
> >  			ai = NULL;
> >  		}
> >  	}
> > +#endif
> >  
> >  	return ai;
> >  }
> > +
> >  #endif	/* !HAVE_GETNAMEINFO */
> >