Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
Somewhat underexplained, given that it seems to add some new
semantics.
> +static void clear_filename(struct lock_file *lk)
> +{
> + free(lk->filename);
> + lk->filename = NULL;
> +}
It is good to abstract out lk->filename[0] = '\0', which used to be
the way we say that we are done with the lock. But I am somewhat
surprised to see that there aren't so many locations that used to
check !!lk->filename[0] to see if we are done with the lock to require
a corresponding wrapper.
> static void remove_lock_file(void)
> {
> pid_t me = getpid();
>
> while (lock_file_list) {
> if (lock_file_list->owner == me &&
> - lock_file_list->filename[0]) {
> + lock_file_list->filename) {
... and this seems to be the only location?
> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)
>
> static int lock_file(struct lock_file *lk, const char *path, int flags)
> {
> - /*
> - * subtract 5 from size to make sure there's room for adding
> - * ".lock" for the lock file name
> - */
> - static const size_t max_path_len = sizeof(lk->filename) - 5;
> -
> - if (strlen(path) >= max_path_len)
> + int len;
> + if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))
> return -1;
Somehow I found it unnecessarily denser; had to read it twice before
caffeine kicked in ;-)
> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)
>
> int commit_lock_file(struct lock_file *lk)
> {
> - char result_file[PATH_MAX];
> - size_t i;
> - if (lk->fd >= 0 && close_lock_file(lk))
> + char *result_file;
> + if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)
> return -1;
We did not protect against somebody calling this with an already
closed lock, but we now return early without attempting renameing
etc., which is a good change but is not explained. Was there a
specific code path that you needed this change for?
Also the order of the check is not consistent with how the same
check is done in rollback_lock_file(). The order you use in this
new code (and also in commit_locked_index()) may be better than the
existing order in the rollback code path; we want to see the fd
closed, if it is open, even if lk->filename has already been
cleared. On the other hand, one could argue that anything such a
broken caller tells this function is suspicious, and we shouldn't
close random file descriptor that is likely not owned by the caller
in the first place. I dunno.
> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)
>
> void rollback_lock_file(struct lock_file *lk)
> {
> - if (lk->filename[0]) {
> + if (lk->filename) {
> if (lk->fd >= 0)
> close(lk->fd);
> unlink_or_warn(lk->filename);
> }
> - lk->filename[0] = 0;
> + clear_filename(lk);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html