bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Paul Eggert]

Thanks, I propagated that into Coreutils and installed the simplified 
patch I mentioned yesterday. Closing the coreutils bug report.

bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Bruno Haible]

I wrote in :
>   - Gnulib modules should better provide .h files that can be #included
> on any platform. Thus, it's Gnulib's task to provide a readutmp.h
> and a read_utmp() function that can also be used on native Windows.

Done as follows:


2023-07-31  Bruno Haible  

readutmp: Make the header file and function usable on all platforms.
* lib/readutmp.h (struct gl_utmp, UTMP_STRUCT_NAME, UT_TIME_MEMBER,
UT_EXIT_E_TERMINATION, UT_EXIT_E_EXIT, UT_USER): Provide fallback
definitions.
(READ_UTMP_SUPPORTED): New macro.
* lib/readutmp.c (read_utmp) [!READ_UTMP_SUPPORTED]: Provide a dummy
definition.
* modules/readutmp (Depends-on): Add sys_time.
(configure.ac): Remove conditional.
(Makefile.am): Compile readutmp.c on all platforms.
(Include): Include readutmp.h on all platforms.
* tests/test-readutmp.c: Include readutmp.h on all platforms.
(main): Invoke read_utmp on all platforms.

diff --git a/lib/readutmp.c b/lib/readutmp.c
index acffe1000e..af43d1ad6b 100644
--- a/lib/readutmp.c
+++ b/lib/readutmp.c
@@ -61,6 +61,8 @@ extract_trimmed_name (const STRUCT_UTMP *ut)
   return trimmed_name;
 }
 
+#if READ_UTMP_SUPPORTED
+
 /* Is the utmp entry U desired by the user who asked for OPTIONS?  */
 
 static bool
@@ -77,12 +79,12 @@ desirable_utmp_entry (STRUCT_UTMP const *u, int options)
   return true;
 }
 
-#ifdef UTMP_NAME_FUNCTION
+# if defined UTMP_NAME_FUNCTION
 
 static void
 copy_utmp_entry (STRUCT_UTMP *dst, STRUCT_UTMP *src)
 {
-#if __GLIBC__ && _TIME_BITS == 64
+#  if __GLIBC__ && _TIME_BITS == 64
   /* Convert from external form in SRC to internal form in DST.
  It is OK to convert now, rather than earlier, before
  desirable_utmp_entry was invoked, because desirable_utmp_entry
@@ -119,9 +121,9 @@ copy_utmp_entry (STRUCT_UTMP *dst, STRUCT_UTMP *src)
   dst->ut_tv.tv_sec = s->ut_tv.tv_sec;
   dst->ut_tv.tv_usec = s->ut_tv.tv_usec;
   memcpy (>ut_addr_v6, s->ut_addr_v6, sizeof dst->ut_addr_v6);
-#else
+#  else
   *dst = *src;
-#endif
+#  endif
 }
 
 int
@@ -158,7 +160,7 @@ read_utmp (char const *file, size_t *n_entries, STRUCT_UTMP 
**utmp_buf,
   return 0;
 }
 
-#else
+# else
 
 int
 read_utmp (char const *file, size_t *n_entries, STRUCT_UTMP **utmp_buf,
@@ -198,4 +200,16 @@ read_utmp (char const *file, size_t *n_entries, 
STRUCT_UTMP **utmp_buf,
   return 0;
 }
 
+# endif
+
+#else /* dummy fallback */
+
+int
+read_utmp (char const *file, size_t *n_entries, STRUCT_UTMP **utmp_buf,
+   int options)
+{
+  errno = ENOSYS;
+  return -1;
+}
+
 #endif
diff --git a/lib/readutmp.h b/lib/readutmp.h
index 7d2a628135..d710699525 100644
--- a/lib/readutmp.h
+++ b/lib/readutmp.h
@@ -38,6 +38,7 @@
 # endif
 
 # if HAVE_UTMPX_H
+
 #  if HAVE_UTMP_H
 /* HPUX 10.20 needs utmp.h, for the definition of e.g., UTMP_FILE.  */
 #   include 
@@ -114,6 +115,24 @@
 #   endif
 #  endif
 
+# else
+
+/* Provide a dummy fallback.  */
+
+/* Get 'struct timeval'.  */
+#  include 
+
+struct gl_utmp
+{
+  char ut_user[1];
+  char ut_line[1];
+  struct timeval ut_tv;
+};
+#  define UTMP_STRUCT_NAME gl_utmp
+#  define UT_TIME_MEMBER(UT_PTR) ((UT_PTR)->ut_tv.tv_sec)
+#  define UT_EXIT_E_TERMINATION(U) 0
+#  define UT_EXIT_E_EXIT(U) 0
+
 # endif
 
 /* Accessor macro for the member named ut_user or ut_name.  */
@@ -137,6 +156,10 @@
 #   define UT_USER(Utmp) ((Utmp)->ut_name)
 #  endif
 
+# else /* dummy fallback */
+
+#  define UT_USER(Utmp) ((Utmp)->ut_user)
+
 # endif
 
 # define HAVE_STRUCT_XTMP_UT_EXIT \
@@ -151,10 +174,14 @@
 (HAVE_STRUCT_UTMP_UT_PID \
  || HAVE_STRUCT_UTMPX_UT_PID)
 
+/* Type of entry returned by read_utmp().  */
 typedef struct UTMP_STRUCT_NAME STRUCT_UTMP;
 
+/* Size of the UT_USER (ut) member.  */
 enum { UT_USER_SIZE = sizeof UT_USER ((STRUCT_UTMP *) 0) };
 
+/* Definition of UTMP_FILE and WTMP_FILE.  */
+
 # if !defined UTMP_FILE && defined _PATH_UTMP
 #  define UTMP_FILE _PATH_UTMP
 # endif
@@ -181,12 +208,15 @@ enum { UT_USER_SIZE = sizeof UT_USER ((STRUCT_UTMP *) 0) 
};
 #  define WTMP_FILE "/etc/wtmp"
 # endif
 
+/* Accessor macro for the member named ut_pid.  */
 # if HAVE_STRUCT_XTMP_UT_PID
 #  define UT_PID(U) ((U)->ut_pid)
 # else
 #  define UT_PID(U) 0
 # endif
 
+/* Accessor macros for the member named ut_type.  */
+
 # if HAVE_STRUCT_UTMP_UT_TYPE || HAVE_STRUCT_UTMPX_UT_TYPE
 #  define UT_TYPE_EQ(U, V) ((U)->ut_type == (V))
 #  define UT_TYPE_NOT_DEFINED 0
@@ -207,11 +237,17 @@ enum { UT_USER_SIZE = sizeof UT_USER ((STRUCT_UTMP *) 0) 
};
 #  define UT_TYPE_USER_PROCESS(U) 0
 # endif
 
+/* Determines whether an entry *U corresponds to a user process.  */
 # define IS_USER_PROCESS(U) \
(UT_USER (U)[0]  \
 && (UT_TYPE_USER_PROCESS (U)\
 || (UT_TYPE_NOT_DEFINED && UT_TIME_MEMBER (U) != 0)))
 

bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Bruno Haible]

Paul Eggert wrote:
> > I'm fine with the change, but we'll also need to adjust
> > the sc_prohibit_always_true_header_tests syntax check in gnulib
> 
> I looked into that but it's such a hassle that I came up with the 
> attached simpler patch to Coreutils. How about installing it instead?

Yes, that's better than what I proposed, on two accounts:
  - Gnulib modules should better provide .h files that can be #included
on any platform. Thus, it's Gnulib's task to provide a readutmp.h
and a read_utmp() function that can also be used on native Windows.
  - It gets rid of the horrible hack to pass the values of two uninitialized
variables down to a function.

Pádraig Brady wrote:
> gnulib docs
> state the following platforms have neither getutent or getutxent,
> and so might have issues with this?

OpenBSD 6.7, Minix 3.1.8, mingw, MSVC 14

On these platforms the #include "readutmp.h" would already provoke a
mass of syntax errors.

Btw, we don't test on Minix any longer (since Minix is dead),
and OpenBSD 6.x is end-of-life already [1].

Bruno

https://en.wikipedia.org/wiki/OpenBSD#Releases

bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Pádraig Brady]

On 30/07/2023 20:43, Paul Eggert wrote:

On 2023-07-30 11:41, Pádraig Brady wrote:

I'm fine with the change, but we'll also need to adjust
the sc_prohibit_always_true_header_tests syntax check in gnulib


I looked into that but it's such a hassle that I came up with the
attached simpler patch to Coreutils. How about installing it instead? No
Gnulib change should be needed.


Maybe, though I notice that gnulib docs
state the following platforms have neither getutent or getutxent,
and so might have issues with this?

OpenBSD 6.7, Minix 3.1.8, mingw, MSVC 14

bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Paul Eggert]

On 2023-07-30 11:41, Pádraig Brady wrote:

I'm fine with the change, but we'll also need to adjust
the sc_prohibit_always_true_header_tests syntax check in gnulib


I looked into that but it's such a hassle that I came up with the 
attached simpler patch to Coreutils. How about installing it instead? No 
Gnulib change should be needed.From 08cb549b6089519c1900339189398e66521b19ea Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sun, 30 Jul 2023 16:30:08 +0200
Subject: [PATCH] uptime: output correct user count on OpenBSD

* src/uptime.c (print_uptime, uptime): Always call read_utmp
and count the result.
* NEWS: Mention the fix (text by Bruno Haible).
---
 NEWS | 3 +++
 src/uptime.c | 7 ---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 92e591ee2..41205fa88 100644
--- a/NEWS
+++ b/NEWS
@@ -46,6 +46,9 @@ GNU coreutils NEWS-*- outline -*-
   'pr --length=1 --double-space' no longer enters an infinite loop.
   [This bug was present in "the beginning".]
 
+  'uptime' no longer incorrectly prints "0 users" on OpenBSD.
+  [bug introduced in coreutils-9.2]
+
   'wc -l' and 'cksum' no longer crash with an "Illegal instruction" error
   on x86 Linux kernels that disable XSAVE YMM.  This was seen on Xen VMs.
   [bug introduced in coreutils-9.0]
diff --git a/src/uptime.c b/src/uptime.c
index bc31026c0..b9d5e3c02 100644
--- a/src/uptime.c
+++ b/src/uptime.c
@@ -100,7 +100,6 @@ print_uptime (size_t n, const STRUCT_UTMP *this)
   }
 #endif
 
-#if HAVE_STRUCT_UTMP_UT_TYPE || HAVE_STRUCT_UTMPX_UT_TYPE
   /* Loop through all the utmp entries we just read and count up the valid
  ones, also in the process possibly gleaning boottime. */
   while (n--)
@@ -110,10 +109,6 @@ print_uptime (size_t n, const STRUCT_UTMP *this)
 boot_time = UT_TIME_MEMBER (this);
   ++this;
 }
-#else
-  (void) n;
-  (void) this;
-#endif
 
   time_now = time (nullptr);
 #if defined HAVE_PROC_UPTIME
@@ -177,10 +172,8 @@ uptime (char const *filename, int options)
   size_t n_users;
   STRUCT_UTMP *utmp_buf = nullptr;
 
-#if HAVE_STRUCT_UTMP_UT_TYPE || HAVE_STRUCT_UTMPX_UT_TYPE
   if (read_utmp (filename, _users, _buf, options) != 0)
 error (EXIT_FAILURE, errno, "%s", quotef (filename));
-#endif
 
   print_uptime (n_users, utmp_buf);
 
-- 
2.39.2

bug#64954: GNU 'uptime' on OpenBSD always prints "0 users"

[Pádraig Brady]

On 30/07/2023 15:44, Bruno Haible wrote:

Hi,

GNU coreutils-9.3 'uptime', on OpenBSD 7.2, prints

  16:24:53  up 14 days 13:33,  0 users,  load average: 0.04, 0.44, 0.59

whereas the OpenBSD /usr/bin/uptime prints

  4:24PM  up 14 days, 13:33, 1 user, load averages: 0.04, 0.44, 0.59

The utmp file contain these entries:

 Time (GMT)   User  PID Term Exit Boot User Process
--- --- --    
1970-01-01 00:00:00  0   00
1970-01-01 00:00:00  0   00
2023-07-16 00:52:00 bruno0   00

The readutmp.h code, in particular the IS_USER_PROCESS macro, works fine.
(UT_TYPE_USER_PROCESS (U) expands to 0, and UT_TYPE_NOT_DEFINED expands to 1.)

The problem is that the readutmp code is entirely disabled on this platform.
Which is not appropriate.

When I revert the coreutils commit 2984e47c789ebc39f55a3b1cb20b943de88eeedc,
the coreutils 'uptime' program prints "1 user", as expected.

The rationale of that commit was wrong:
"Following gnulib commit 9041103 HAVE_UTMP_H will always be defined."
No, what that gnulib commit did is to create an  replacement
if the package imports the 'login_tty' module. But the HAVE_UTMP_H
is *still* only defined if the platform provides it.
Proof: On FreeBSD 13.2, which does not have  (it has 
instead), configuring coreutils-9.3 produces the attached config.h,
which has

#define HAVE_UTMPX_H 1
/* #undef HAVE_UTMP_H */

So, find attached the fix of the bug.


I'm fine with the change, but we'll also need to adjust
the sc_prohibit_always_true_header_tests syntax check in gnulib
as that will fail since gnulib/lib/utmp.in.h is present
and thus use of HAVE_UTMP_H is disallowed.

cheers,
Pádraig