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

Message ID 87ef1qvhak.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • login: Replace macro-based control flow with function calls in utmp
Related show

Commit Message

Florian Weimer Aug. 12, 2019, 9:56 a.m. UTC
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.

Comments

Adhemerval Zanella Aug. 12, 2019, 5:48 p.m. UTC | #1
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.
Florian Weimer Aug. 12, 2019, 7:55 p.m. UTC | #2
* Adhemerval Zanella:

>> +/* 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.

Sorry, I don't see it.  How so?

>> +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' ?

The name reflects the use, like this:

>> +  struct file_locking fl;
>> +  if (file_locking_failed (&fl, file_fd, F_RDLCK))
>> +    nbytes = 0;
>> +  else
>>      {
>> +      /* Read the next entry.  */
>> +      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
>> +      file_locking_unlock (file_fd);
>>      }

Should I call it try_file_lock, still with true for the failure case?

Thanks,
Florian
Adhemerval Zanella Aug. 12, 2019, 8:05 p.m. UTC | #3
On 12/08/2019 16:55, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> +/* 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.
> 
> Sorry, I don't see it.  How so?

In fact the period inside the inside parentheses seems ok, sorry for noise.

> 
>>> +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' ?
> 
> The name reflects the use, like this:
> 
>>> +  struct file_locking fl;
>>> +  if (file_locking_failed (&fl, file_fd, F_RDLCK))
>>> +    nbytes = 0;
>>> +  else
>>>      {
>>> +      /* Read the next entry.  */
>>> +      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
>>> +      file_locking_unlock (file_fd);
>>>      }
> 
> Should I call it try_file_lock, still with true for the failure case?

I think it sounds more straightforward.  Usually on other internal APIs
the 'failed' keywork is used on function is actually checking for failure
(for instance 'alloc_buffer_has_failed'), instead of a function that 
executes the command as well.

> 
> Thanks,
> Florian
>
Florian Weimer Aug. 13, 2019, 10:55 a.m. UTC | #4
* Adhemerval Zanella:

>>>> +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' ?
>> 
>> The name reflects the use, like this:
>> 
>>>> +  struct file_locking fl;
>>>> +  if (file_locking_failed (&fl, file_fd, F_RDLCK))
>>>> +    nbytes = 0;
>>>> +  else
>>>>      {
>>>> +      /* Read the next entry.  */
>>>> +      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
>>>> +      file_locking_unlock (file_fd);
>>>>      }
>> 
>> Should I call it try_file_lock, still with true for the failure case?
>
> I think it sounds more straightforward.  Usually on other internal APIs
> the 'failed' keywork is used on function is actually checking for failure
> (for instance 'alloc_buffer_has_failed'), instead of a function that 
> executes the command as well.

Fair enough.  Here's a new version of the patch, with renamed functions.

Thanks,
Florian

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

2019-08-05  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.

diff --git a/login/utmp_file.c b/login/utmp_file.c
index 9badf11fb3..7bd6034af4 100644
--- 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);
Adhemerval Zanella Aug. 13, 2019, 1:47 p.m. UTC | #5
On 13/08/2019 07:55, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>>> +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' ?
>>>
>>> The name reflects the use, like this:
>>>
>>>>> +  struct file_locking fl;
>>>>> +  if (file_locking_failed (&fl, file_fd, F_RDLCK))
>>>>> +    nbytes = 0;
>>>>> +  else
>>>>>      {
>>>>> +      /* Read the next entry.  */
>>>>> +      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
>>>>> +      file_locking_unlock (file_fd);
>>>>>      }
>>>
>>> Should I call it try_file_lock, still with true for the failure case?
>>
>> I think it sounds more straightforward.  Usually on other internal APIs
>> the 'failed' keywork is used on function is actually checking for failure
>> (for instance 'alloc_buffer_has_failed'), instead of a function that 
>> executes the command as well.
> 
> Fair enough.  Here's a new version of the patch, with renamed functions.
> 
> Thanks,
> Florian
> 
> login: Replace macro-based control flow with function calls in utmp
> 
> 2019-08-05  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.

LGTM, thanks.

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

> 
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 9badf11fb3..7bd6034af4 100644
> --- 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);
>

Patch
diff mbox series

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.)
+
+   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;
+}
+
+static void
+file_locking_unlock (int fd)
+{
+  struct flock fl =
+    {
+      .l_type = F_UNLCK,
+    };
+  __fcntl64_nocancel (fd, F_SETLKW, &fl);
+}
+
+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);
+}
 
 #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))
     {
@@ -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;
     }
 
   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;
 }
@@ -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)
@@ -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);
 }
@@ -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)
@@ -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;
 }
@@ -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);
@@ -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);