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 <sahlb...@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;
> +             }

Wouldn't it make sense for error() to save and restore errno instead of
scattering the save/restore code around everywhere?  I saw the same type
of code about three commits later, too.

> [...]

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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