Discussion:
[PATCH] mountlist/ptsname_r: leverage AC_HEADER_MAJOR
(too old to reply)
Mike Frysinger
2016-04-16 06:11:37 UTC
Permalink
Raw Message
These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.

* lib/mountlist.c [MAJOR_IN_MKDEV]: Include sys/mkdev.h.
[MAJOR_IN_SYSMACROS]: Include sys/sysmacros.h.
* lib/ptsname_r.c: Likewise.
[__sun]: Delete sys/sysmacros.h include.
[_AIX || __osf__]: Likewise.
* m4/mountlist.m4 (gl_MOUNTLIST): Require AC_HEADER_MAJOR.
* m4/ptsname_r.m4 (gl_FUNC_PTSNAME_R): Likewise.
---
Note: Untested :).

lib/mountlist.c | 7 +++++++
lib/ptsname_r.c | 12 ++++++++----
m4/mountlist.m4 | 1 +
m4/ptsname_r.m4 | 2 ++
4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/lib/mountlist.c b/lib/mountlist.c
index bb4e4ee..f3e7f22 100644
--- a/lib/mountlist.c
+++ b/lib/mountlist.c
@@ -37,6 +37,13 @@
# include <sys/param.h>
#endif

+#if MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#endif
+#if MAJOR_IN_SYSMACROS
+# include <sys/sysmacros.h>
+#endif
+
#if defined MOUNTED_GETFSSTAT /* OSF_1 and Darwin1.3.x */
# if HAVE_SYS_UCRED_H
# include <grp.h> /* needed on OSF V4.0 for definition of NGROUPS,
diff --git a/lib/ptsname_r.c b/lib/ptsname_r.c
index 14c36d6..b67270d 100644
--- a/lib/ptsname_r.c
+++ b/lib/ptsname_r.c
@@ -42,22 +42,26 @@

#endif

+/* Get the major, minor macros. */
+#if MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#endif
+#if MAJOR_IN_SYSMACROS
+# include <sys/sysmacros.h>
+#endif
+
#ifdef __sun
/* Get ioctl() and 'struct strioctl'. */
# include <stropts.h>
/* Get ISPTM. */
# include <sys/stream.h>
# include <sys/ptms.h>
-/* Get the major, minor macros. */
-# include <sys/sysmacros.h>
# include <stdio.h>
#endif

#if defined _AIX || defined __osf__
/* Get ioctl(), ISPTM. */
# include <sys/ioctl.h>
-/* Get the major, minor macros. */
-# include <sys/sysmacros.h>
# include <stdio.h>
#endif

diff --git a/m4/mountlist.m4 b/m4/mountlist.m4
index 2e2ca37..24f49b8 100644
--- a/m4/mountlist.m4
+++ b/m4/mountlist.m4
@@ -6,6 +6,7 @@ dnl with or without modifications, as long as this notice is preserved.

AC_DEFUN([gl_MOUNTLIST],
[
+ AC_REQUIRE([AC_HEADER_MAJOR])
gl_LIST_MOUNTED_FILE_SYSTEMS([gl_cv_list_mounted_fs=yes],
[gl_cv_list_mounted_fs=no])
])
diff --git a/m4/ptsname_r.m4 b/m4/ptsname_r.m4
index 4ddde81..d71cac4 100644
--- a/m4/ptsname_r.m4
+++ b/m4/ptsname_r.m4
@@ -42,6 +42,8 @@ AC_DEFUN([gl_FUNC_PTSNAME_R],
REPLACE_PTSNAME_R=1
fi
fi
+
+ AC_REQUIRE([AC_HEADER_MAJOR])
])

# Prerequisites of lib/ptsname.c.
--
2.7.4
Bruno Haible
2016-04-16 10:51:45 UTC
Permalink
Raw Message
Hi Mike,
Post by Mike Frysinger
These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.
Does this change fix a compilation problem? You did not say so.

Does this change make future maintenance easier? I don't think so.
mountlist.c and ptsname_r.c are quite special among Gnulib code:
they have #if sections for each particular platform. For example,
the ptsname_r.c file has
- a code section for Solaris, that uses major, minor,
- an #include section for Solaris, that includes the appropriate
header,
- a code section for AIX and OSF/1, that uses minor,
- an #include section for AIX and OSF/1, that includes the appropriate
header.
This is all consistent.

Solaris, AIX, OSF/1 won't change much in the future. I don't expect
that they will restructure their header files, in the way actively
developed platforms occasionally do.

There is also little chance that one will want to reuse the code for
Solaris, AIX, OSF/1 for any new platform. (GNU/Solaris? Not in
development.)

Similarly for mountlist.c.

So, all I can see is that your change will make configure times longer,
in particular on glibc and BSD systems, for no real purpose.

We have not split mountlist.c and ptsname_r.c into specific files, one
per platform:
ptsname_r-solaris.c
ptsname_r-aix-osf.c
ptsname_r-generic.c
because we thought that maintenance is easier with a single file. Maybe
we should have done it?

Bruno
Pádraig Brady
2016-04-16 12:20:32 UTC
Permalink
Raw Message
Post by Bruno Haible
Hi Mike,
Post by Mike Frysinger
These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.
Does this change fix a compilation problem? You did not say so.
Context in http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00025.html
glibc-2.23 and musl now need this change it seems.

I suggested a makedev gnulib module in the thread above
as it's not just the headers that are varying,
as also some systems use mkdev() and some makedev().

The patch should be fine to apply though and fixes an immediate issue.

thanks,
Pádraig
Dmitry V. Levin
2016-04-16 13:20:27 UTC
Permalink
Raw Message
Post by Pádraig Brady
Post by Bruno Haible
Hi Mike,
Post by Mike Frysinger
These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.
Does this change fix a compilation problem? You did not say so.
Context in
http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00025.html
glibc-2.23 and musl now need this change it seems.
I suggested a makedev gnulib module in the thread above
as it's not just the headers that are varying,
as also some systems use mkdev() and some makedev().
The patch should be fine to apply though and fixes an immediate issue.
Only mountlist part of the patch is relevant, the ptsname_r part is not
needed.
--
ldv
Mike Frysinger
2016-04-17 02:17:18 UTC
Permalink
Raw Message
Post by Pádraig Brady
Post by Bruno Haible
Post by Mike Frysinger
These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.
Does this change fix a compilation problem? You did not say so.
Context in http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00025.html
glibc-2.23 and musl now need this change it seems.
heh, i completely forgot about that thread ;)
-mike
Bruno Haible
2016-12-02 00:40:45 UTC
Permalink
Raw Message
Pádraig Brady wrote in http://lists.gnu.org/archive/html/bug-gnulib/2016-04/msg00022.html
Post by Pádraig Brady
Context in http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00025.html
glibc-2.23 and musl now need this change it seems.
The patch should be fine to apply though and fixes an immediate issue.
glibc and musl cannot need a change that only affects the behaviour on
Solaris, AIX, and OSF/1. => There was no "immediate issue".

Actually the only benefit of the change is that on Solaris, the code now
uses <sys/mkdev.h> instead of <sys/sysmacros.h>. <sys/mkdev.h> is somewhat
cleaner (doesn't define unrelated old cruft).

Bruno
Mike Frysinger
2016-12-06 04:10:07 UTC
Permalink
Raw Message
Pádraig Brady wrote in http://lists.gnu.org/archive/html/bug-gnulib/2016-04/msg00022.html
Post by Pádraig Brady
Context in http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00025.html
glibc-2.23 and musl now need this change it seems.
The patch should be fine to apply though and fixes an immediate issue.
glibc and musl cannot need a change that only affects the behaviour on
Solaris, AIX, and OSF/1. => There was no "immediate issue".
Actually the only benefit of the change is that on Solaris, the code now
uses <sys/mkdev.h> instead of <sys/sysmacros.h>. <sys/mkdev.h> is somewhat
cleaner (doesn't define unrelated old cruft).
i was just grepping for all users and bulk converting them over. you're
certainly correct in the case of the ptsname_r module, the current code
that calls major/minor isn't used on glibc/musl/etc... systems.
-mike

Bruno Haible
2016-12-02 00:40:56 UTC
Permalink
Raw Message
Post by Pádraig Brady
I suggested a makedev gnulib module in the thread above
as it's not just the headers that are varying,
as also some systems use mkdev() and some makedev().
http://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00023.html
I agree that gnulib is a good place to handle this. Some more info:

Platforms that have makedev as a function: none.

Platforms that have makedev as a macro:
in <sys/mkdev.h>: glibc, IRIX, Solaris, Interix
in <sys/sysmacros.h>: AIX, BeOS, Cygwin, HP-UX, IRIX, Solaris
in <sys/types.h>: FreeBSD, NetBSD, OpenBSD, Mac OS X, Minix, OSF/1

Platforms that have mkdev as a function: none

Platforms that have mkdev as a macro:
in <sys/mkdev.h>: Interix

Bruno
Mike Frysinger
2016-11-27 03:52:00 UTC
Permalink
Raw Message
The mountlist module changes were merged independently since my v1,
but the ptsname module still hasn't been updated.

These two modules use makedev/major/minor but don't have explicit
checks for the functions. Use the existing autoconf macro which
will probe some headers for use and set up some defines.

* lib/ptsname_r.c: Likewise.
[__sun]: Delete sys/sysmacros.h include.
[_AIX || __osf__]: Likewise.
* m4/ptsname_r.m4 (gl_FUNC_PTSNAME_R): Likewise.
---
lib/ptsname_r.c | 12 ++++++++----
m4/ptsname_r.m4 | 2 ++
2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/ptsname_r.c b/lib/ptsname_r.c
index f4887bc97fb9..e0c634e20f7c 100644
--- a/lib/ptsname_r.c
+++ b/lib/ptsname_r.c
@@ -47,22 +47,26 @@

#endif

+/* Get the major, minor macros. */
+#if MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#endif
+#if MAJOR_IN_SYSMACROS
+# include <sys/sysmacros.h>
+#endif
+
#ifdef __sun
/* Get ioctl() and 'struct strioctl'. */
# include <stropts.h>
/* Get ISPTM. */
# include <sys/stream.h>
# include <sys/ptms.h>
-/* Get the major, minor macros. */
-# include <sys/sysmacros.h>
# include <stdio.h>
#endif

#if defined _AIX || defined __osf__
/* Get ioctl(), ISPTM. */
# include <sys/ioctl.h>
-/* Get the major, minor macros. */
-# include <sys/sysmacros.h>
# include <stdio.h>
#endif

diff --git a/m4/ptsname_r.m4 b/m4/ptsname_r.m4
index 4ddde81ee487..d71cac422aff 100644
--- a/m4/ptsname_r.m4
+++ b/m4/ptsname_r.m4
@@ -42,6 +42,8 @@ AC_DEFUN([gl_FUNC_PTSNAME_R],
REPLACE_PTSNAME_R=1
fi
fi
+
+ AC_REQUIRE([AC_HEADER_MAJOR])
])

# Prerequisites of lib/ptsname.c.
--
2.11.0.rc2
Pádraig Brady
2016-11-27 11:42:45 UTC
Permalink
Raw Message
Pushed.

thanks
Loading...