Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
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
> ---
> 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
Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
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
> ---
> 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
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
[PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
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
---
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);
--
2.0.0.438.gec92e5c
--
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
