Nguyễn Thái Ngọc Duy  <pclo...@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to