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

Adhemerval Zanella adhemerval.zanella@linaro.org
Mon Aug 12 17:48:00 GMT 2019



On 12/08/2019 06:56, Florian Weimer wrote:
> 2019-08-05  Florian Weimer  <fweimer@redhat.com>
> 
> 	* login/utmp_file.c (LOCK_FILE, LOCKING_FAILED, UNLOCK_FILE):
> 	Remove macros.
> 	(struct file_locking): New.
> 	(file_locking_failed, file_locking_unlock, file_locking_restore):
> 	New function.
> 	(__libc_getutent_r): Use the new functions.
> 	(internal_getut_r): Likewise.
> 	(__libc_getutline_r): Likewise.
> 	(__libc_pututline): Likewise.
> 	(__libc_updwtmp): Likewise.

LGTM, with a nit below.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 9badf11fb3..3f21de2b71 100644
> --- a/login/utmp_file.c
> +++ b/login/utmp_file.c
> @@ -52,58 +52,72 @@ 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)
> +
> +
> +/* file_locking_failed (LOCKING, FD, TYPE) returns true if the locking
> +   operation failed and recovery needs to be performed.
> +   (file_locking_restore (LOCKING) still needs to be called.)

Punctuation seems strange here.

> +
> +   file_locking_unlock (FD) removes the lock (which must have been
> +   acquired).
> +
> +   file_locking_restore (LOCKING) is needed to clean up in both
> +   cases.  */
> +
> +struct file_locking
> +{
> +  struct sigaction old_action;
> +  unsigned int old_timeout;
> +};
> +
> +static bool
> +file_locking_failed (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;
> +}

The name with ending '_failed' sound confusing, why not just 'file_locking_bool' ?

> +
> +static void
> +file_locking_unlock (int fd)
> +{
> +  struct flock fl =
> +    {
> +      .l_type = F_UNLCK,
> +    };
> +  __fcntl64_nocancel (fd, F_SETLKW, &fl);
> +}
> +

Ok.

> +static void
> +file_locking_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);
> +}
>  

Ok.

>  #ifndef TRANSFORM_UTMP_FILE_NAME
>  # define TRANSFORM_UTMP_FILE_NAME(file_name) (file_name)
> @@ -153,16 +167,16 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
>        return -1;
>      }
>  
> -  LOCK_FILE (file_fd, F_RDLCK)
> +  struct file_locking fl;
> +  if (file_locking_failed (&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_locking_unlock (file_fd);
>      }
> -
> -  /* Read the next entry.  */
> -  nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
> -
> -  UNLOCK_FILE (file_fd);
> +  file_locking_restore (&fl);
>  
>    if (nbytes != sizeof (struct utmp))
>      {

Ok.

> @@ -188,10 +202,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 (file_locking_failed (&fl, file_fd, F_RDLCK))
>      {
>        *lock_failed = true;
> -      LOCKING_FAILED ();
> +      file_locking_restore (&fl);
> +      return -1;
>      }
>  

Ok.

>    if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME
> @@ -241,7 +257,8 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
>    result = 0;
>  
>  unlock_return:
> -  UNLOCK_FILE (file_fd);
> +  file_locking_unlock (file_fd);
> +  file_locking_restore (&fl);
>  
>    return result;
>  }

Ok.

> @@ -287,10 +304,12 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
>        return -1;
>      }
>  
> -  LOCK_FILE (file_fd, F_RDLCK)
> +  struct file_locking fl;
> +  if (file_locking_failed (&fl, file_fd, F_RDLCK))
>      {
>        *result = NULL;
> -      LOCKING_FAILED ();
> +      file_locking_restore (&fl);
> +      return -1;
>      }
>  
>    while (1)

Ok.

> @@ -318,7 +337,8 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
>    *result = buffer;
>  
>  unlock_return:
> -  UNLOCK_FILE (file_fd);
> +  file_locking_unlock (file_fd);
> +  file_locking_restore (&fl);
>  
>    return ((*result == NULL) ? -1 : 0);
>  }

Ok.

> @@ -375,10 +395,11 @@ __libc_pututline (const struct utmp *data)
>  	}
>      }
>  
> -  LOCK_FILE (file_fd, F_WRLCK)
> +  struct file_locking fl;
> +  if (file_locking_failed (&fl, file_fd, F_WRLCK))
>      {
> -      pbuf = NULL;
> -      LOCKING_FAILED ();
> +      file_locking_restore (&fl);
> +      return NULL;
>      }
>  
>    if (found < 0)

Ok.

> @@ -421,7 +442,8 @@ __libc_pututline (const struct utmp *data)
>      }
>  
>   unlock_return:
> -  UNLOCK_FILE (file_fd);
> +  file_locking_unlock (file_fd);
> +  file_locking_restore (&fl);
>  
>    return pbuf;
>  }

Ok.

> @@ -450,8 +472,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 (file_locking_failed (&fl, fd, F_WRLCK))
> +    {
> +      file_locking_restore (&fl);
> +      __close_nocancel_nostatus (fd);
> +      return -1;
> +    }
>  
>    /* Remember original size of log file.  */
>    offset = __lseek64 (fd, 0, SEEK_END);

Ok.

> @@ -477,7 +504,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
>    result = 0;
>  
>  unlock_return:
> -  UNLOCK_FILE (fd);
> +  file_locking_unlock (file_fd);
> +  file_locking_restore (&fl);
>  
>    /* Close WTMP file.  */
>    __close_nocancel_nostatus (fd);
> 

Ok.



More information about the Libc-alpha mailing list