login: Replace macro-based control flow with function calls in utmp

diff --git a/ChangeLog b/ChangeLog
index f06bed449b8d8033235bab2a6574dcc22174cc2e..71b254158ddb9f4b1069b28baab6952cef9a37f5 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2019-08-13  Florian Weimer  <fweimer@redhat.com>
+
+       * login/utmp_file.c (LOCK_FILE, LOCKING_FAILED, UNLOCK_FILE):
+       Remove macros.
+       (struct file_locking): New.
+       (try_file_lock, file_unlock, file_lock_restore): New functions.
+       (__libc_getutent_r): Use the new functions.
+       (internal_getut_r): Likewise.
+       (__libc_getutline_r): Likewise.
+       (__libc_pututline): Likewise.
+       (__libc_updwtmp): Likewise.
+
 2019-08-13  Joseph Myers  <joseph@codesourcery.com>
 
        * bits/libc-header-start.h (__GLIBC_USE_IEC_60559_BFP_EXT): Update

diff --git a/login/utmp_file.c b/login/utmp_file.c
index 9badf11fb3de08b7f5f367130a2260597b272027..7bd6034af47fa782328dd4735e9e9285c710933c 100644 (file)
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -52,58 +52,71 @@ static struct utmp last_entry;
 /* Do-nothing handler for locking timeout.  */
 static void timeout_handler (int signum) {};
 
-/* LOCK_FILE(fd, type) failure_statement
-     attempts to get a lock on the utmp file referenced by FD.  If it fails,
-     the failure_statement is executed, otherwise it is skipped.
-   LOCKING_FAILED()
-     jumps into the UNLOCK_FILE macro and ensures cleanup of LOCK_FILE.
-   UNLOCK_FILE(fd)
-     unlocks the utmp file referenced by FD and performs the cleanup of
-     LOCK_FILE.
- */
-#define LOCK_FILE(fd, type) \
-{                                                                            \
-  struct flock fl;                                                           \
-  struct sigaction action, old_action;                                       \
-  unsigned int old_timeout;                                                  \
-                                                                             \
-  /* Cancel any existing alarm.  */                                          \
-  old_timeout = alarm (0);                                                   \
-                                                                             \
-  /* Establish signal handler.  */                                           \
-  action.sa_handler = timeout_handler;                                       \
-  __sigemptyset (&action.sa_mask);                                           \
-  action.sa_flags = 0;                                                       \
-  __sigaction (SIGALRM, &action, &old_action);                               \
-                                                                             \
-  alarm (TIMEOUT);                                                           \
-                                                                             \
-  /* Try to get the lock.  */                                                \
-  memset (&fl, '\0', sizeof (struct flock));                                 \
-  fl.l_type = (type);                                                        \
-  fl.l_whence = SEEK_SET;                                                    \
-  if (__fcntl64_nocancel ((fd), F_SETLKW, &fl) < 0)
-
-#define LOCKING_FAILED() \
-  goto unalarm_return
-
-#define UNLOCK_FILE(fd) \
-  /* Unlock the file.  */                                                    \
-  fl.l_type = F_UNLCK;                                                       \
-  __fcntl64_nocancel ((fd), F_SETLKW, &fl);                                  \
-                                                                             \
- unalarm_return:                                                             \
-  /* Reset the signal handler and alarm.  We must reset the alarm            \
-     before resetting the handler so our alarm does not generate a           \
-     spurious SIGALRM seen by the user.  However, we cannot just set         \
-     the user's old alarm before restoring the handler, because then         \
-     it's possible our handler could catch the user alarm's SIGARLM          \
-     and then the user would never see the signal he expected.  */           \
-  alarm (0);                                                                 \
-  __sigaction (SIGALRM, &old_action, NULL);                                  \
-  if (old_timeout != 0)                                                              \
-    alarm (old_timeout);                                                     \
-} while (0)
+
+/* try_file_lock (LOCKING, FD, TYPE) returns true if the locking
+   operation failed and recovery needs to be performed.
+   (file_lock_restore (LOCKING) still needs to be called.)
+
+   file_unlock (FD) removes the lock (which must have been
+   acquired).
+
+   file_lock_restore (LOCKING) is needed to clean up in both
+   cases.  */
+
+struct file_locking
+{
+  struct sigaction old_action;
+  unsigned int old_timeout;
+};
+
+static bool
+try_file_lock (struct file_locking *locking, int fd, int type)
+{
+  /* Cancel any existing alarm.  */
+  locking->old_timeout = alarm (0);
+
+  /* Establish signal handler.  */
+  struct sigaction action;
+  action.sa_handler = timeout_handler;
+  __sigemptyset (&action.sa_mask);
+  action.sa_flags = 0;
+  __sigaction (SIGALRM, &action, &locking->old_action);
+
+  alarm (TIMEOUT);
+
+  /* Try to get the lock.  */
+ struct flock fl =
+   {
+    .l_type = type,
+    fl.l_whence = SEEK_SET,
+   };
+ return __fcntl64_nocancel (fd, F_SETLKW, &fl) < 0;
+}
+
+static void
+file_unlock (int fd)
+{
+  struct flock fl =
+    {
+      .l_type = F_UNLCK,
+    };
+  __fcntl64_nocancel (fd, F_SETLKW, &fl);
+}
+
+static void
+file_lock_restore (struct file_locking *locking)
+{
+  /* Reset the signal handler and alarm.  We must reset the alarm
+     before resetting the handler so our alarm does not generate a
+     spurious SIGALRM seen by the user.  However, we cannot just set
+     the user's old alarm before restoring the handler, because then
+     it's possible our handler could catch the user alarm's SIGARLM
+     and then the user would never see the signal he expected.  */
+  alarm (0);
+  __sigaction (SIGALRM, &locking->old_action, NULL);
+  if (locking->old_timeout != 0)
+    alarm (locking->old_timeout);
+}
 
 #ifndef TRANSFORM_UTMP_FILE_NAME
 # define TRANSFORM_UTMP_FILE_NAME(file_name) (file_name)
@@ -153,16 +166,16 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
       return -1;
     }
 
-  LOCK_FILE (file_fd, F_RDLCK)
+  struct file_locking fl;
+  if (try_file_lock (&fl, file_fd, F_RDLCK))
+    nbytes = 0;
+  else
     {
-      nbytes = 0;
-      LOCKING_FAILED ();
+      /* Read the next entry.  */
+      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
+      file_unlock (file_fd);
     }
-
-  /* Read the next entry.  */
-  nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
-
-  UNLOCK_FILE (file_fd);
+  file_lock_restore (&fl);
 
   if (nbytes != sizeof (struct utmp))
     {
@@ -188,10 +201,12 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
 {
   int result = -1;
 
-  LOCK_FILE (file_fd, F_RDLCK)
+  struct file_locking fl;
+  if (try_file_lock (&fl, file_fd, F_RDLCK))
     {
       *lock_failed = true;
-      LOCKING_FAILED ();
+      file_lock_restore (&fl);
+      return -1;
     }
 
   if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME
@@ -241,7 +256,8 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
   result = 0;
 
 unlock_return:
-  UNLOCK_FILE (file_fd);
+  file_unlock (file_fd);
+  file_lock_restore (&fl);
 
   return result;
 }
@@ -287,10 +303,12 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
       return -1;
     }
 
-  LOCK_FILE (file_fd, F_RDLCK)
+  struct file_locking fl;
+  if (try_file_lock (&fl, file_fd, F_RDLCK))
     {
       *result = NULL;
-      LOCKING_FAILED ();
+      file_lock_restore (&fl);
+      return -1;
     }
 
   while (1)
@@ -318,7 +336,8 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
   *result = buffer;
 
 unlock_return:
-  UNLOCK_FILE (file_fd);
+  file_unlock (file_fd);
+  file_lock_restore (&fl);
 
   return ((*result == NULL) ? -1 : 0);
 }
@@ -375,10 +394,11 @@ __libc_pututline (const struct utmp *data)
        }
     }
 
-  LOCK_FILE (file_fd, F_WRLCK)
+  struct file_locking fl;
+  if (try_file_lock (&fl, file_fd, F_WRLCK))
     {
-      pbuf = NULL;
-      LOCKING_FAILED ();
+      file_lock_restore (&fl);
+      return NULL;
     }
 
   if (found < 0)
@@ -421,7 +441,8 @@ __libc_pututline (const struct utmp *data)
     }
 
  unlock_return:
-  UNLOCK_FILE (file_fd);
+  file_unlock (file_fd);
+  file_lock_restore (&fl);
 
   return pbuf;
 }
@@ -450,8 +471,13 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
   if (fd < 0)
     return -1;
 
-  LOCK_FILE (fd, F_WRLCK)
-    LOCKING_FAILED ();
+  struct file_locking fl;
+  if (try_file_lock (&fl, fd, F_WRLCK))
+    {
+      file_lock_restore (&fl);
+      __close_nocancel_nostatus (fd);
+      return -1;
+    }
 
   /* Remember original size of log file.  */
   offset = __lseek64 (fd, 0, SEEK_END);
@@ -477,7 +503,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
   result = 0;
 
 unlock_return:
-  UNLOCK_FILE (fd);
+  file_unlock (file_fd);
+  file_lock_restore (&fl);
 
   /* Close WTMP file.  */
   __close_nocancel_nostatus (fd);