Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 1, 2014, 16:53 UTC
- Message-ID
- <xmqqfvhgw3q9.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1406814214-21725-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Somewhat underexplained, given that it seems to add some new semantics.
Show 5 quoted lines
> +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.
Show 8 quoted lines
> 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?
Show 14 quoted lines
> @@ -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 ;-)
Show 10 quoted lines
> @@ -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.
Show 13 quoted lines
> @@ -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);
> }