* [PATCH] login: Replace macro-based control flow with function calls in utmp
@ 2019-08-12 9:56 Florian Weimer
2019-08-12 17:48 ` Adhemerval Zanella
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2019-08-12 9:56 UTC (permalink / raw)
To: libc-alpha
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.
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
2019-08-12 9:56 [PATCH] login: Replace macro-based control flow with function calls in utmp Florian Weimer
@ 2019-08-12 17:48 ` Adhemerval Zanella
2019-08-12 19:55 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Adhemerval Zanella @ 2019-08-12 17:48 UTC (permalink / raw)
To: libc-alpha
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
2019-08-12 17:48 ` Adhemerval Zanella
@ 2019-08-12 19:55 ` Florian Weimer
2019-08-12 20:05 ` Adhemerval Zanella
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2019-08-12 19:55 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
2019-08-12 19:55 ` Florian Weimer
@ 2019-08-12 20:05 ` Adhemerval Zanella
2019-08-13 10:56 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Adhemerval Zanella @ 2019-08-12 20:05 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
2019-08-12 20:05 ` Adhemerval Zanella
@ 2019-08-13 10:56 ` Florian Weimer
2019-08-13 13:47 ` Adhemerval Zanella
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2019-08-13 10:56 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] login: Replace macro-based control flow with function calls in utmp
2019-08-13 10:56 ` Florian Weimer
@ 2019-08-13 13:47 ` Adhemerval Zanella
0 siblings, 0 replies; 6+ messages in thread
From: Adhemerval Zanella @ 2019-08-13 13:47 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-13 13:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 9:56 [PATCH] login: Replace macro-based control flow with function calls in utmp Florian Weimer
2019-08-12 17:48 ` Adhemerval Zanella
2019-08-12 19:55 ` Florian Weimer
2019-08-12 20:05 ` Adhemerval Zanella
2019-08-13 10:56 ` Florian Weimer
2019-08-13 13:47 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).