Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei

[Michael Haggerty <mhagger@alum.mit.edu>]

There's a typo in the subject line of this commit.

Michael

On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> Making errno when returning from lock_file() meaningful, which should
> fix
> 
>  * an existing almost-bug in lock_ref_sha1_basic where it assumes
>    errno==ENOENT is meaningful and could waste some work on retries
> 
>  * an existing bug in repack_without_refs where it prints
>    strerror(errno) and picks advice based on errno, despite errno
>    potentially being zero and potentially having been clobbered by
>    that point
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  lockfile.c | 17 ++++++++++++-----
>  refs.c     |  1 +
>  refs.h     |  1 +
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/lockfile.c b/lockfile.c
> index 464031b..a921d77 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
>  	return p;
>  }
>  
> -
> +/* Make sure errno contains a meaningful value on error */
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
>  	/*
> @@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  	 */
>  	static const size_t max_path_len = sizeof(lk->filename) - 5;
>  
> -	if (strlen(path) >= max_path_len)
> +	if (strlen(path) >= max_path_len) {
> +		errno = ENAMETOOLONG;
>  		return -1;
> +	}
>  	strcpy(lk->filename, path);
>  	if (!(flags & LOCK_NODEREF))
>  		resolve_symlink(lk->filename, max_path_len);
> @@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  			lock_file_list = lk;
>  			lk->on_list = 1;
>  		}
> -		if (adjust_shared_perm(lk->filename))
> -			return error("cannot fix permission bits on %s",
> -				     lk->filename);
> +		if (adjust_shared_perm(lk->filename)) {
> +			int save_errno = errno;
> +			error("cannot fix permission bits on %s",
> +			      lk->filename);
> +			errno = save_errno;
> +			return -1;
> +		}
>  	}
>  	else
>  		lk->filename[0] = 0;
> @@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)
>  	die("%s", buf.buf);
>  }
>  
> +/* This should return a meaningful errno on failure */
>  int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
>  {
>  	int fd = lock_file(lk, path, flags);
> diff --git a/refs.c b/refs.c
> index db05602..e9d53e4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> +/* This should return a meaningful errno on failure */
>  int lock_packed_refs(int flags)
>  {
>  	struct packed_ref_cache *packed_ref_cache;
> diff --git a/refs.h b/refs.h
> index 09d3564..64f25d9 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -82,6 +82,7 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
>  /*
>   * Lock the packed-refs file for writing.  Flags is passed to
>   * hold_lock_file_for_update().  Return 0 on success.
> + * Errno is set to something meaningful on error.
>   */
>  extern int lock_packed_refs(int flags);
>  
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/