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 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
>