| Message ID | 87ef1qvhak.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | login: Replace macro-based control flow with function calls in utmp | expand |
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.
* 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
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 >
* 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);
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); >
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);