This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
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);
>