Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs
On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Hi,
Comments from http://marc.info/?l=gitm=140079653930751w=2:
Ronnie Sahlberg wrote:
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
unable_to_lock_strbuf could be called unable_to_lock_message (which
would make its behavior more obvious IMHO).
Renamed. Thanks.
[...]
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
This is about making errno meaningful when commit_packed_refs returns
an error. Probably its API documentation should say so to make it
obvious when people modify it in the future that they should preserve
that property or audit callers.
[...]
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path(packed-refs), errno,
+ err);
+ return -1;
+ }
unable_to_lock_error(git_path(packed-refs), errno);
Via the new call to unable_to_lock_..., repack_without_refs cares
about errno after a failed call to lock_packed_refs. lock_packed_refs
can only fail in hold_lock_file_for_update. hold_lock_file_for_update
is a thin wrapper around lockfile.c::lock_file. lock_file can error
out because
strlen(path) = max_path_len
adjust_shared_perm failed [calls error(), clobbers errno]
open failed
So lock_file needs a similar kind of fix, and it's probably worth
updating API documentation for these calls to make it clear that their
errno is used (though that's not a new problem since
repack_without_refs already called unable_to_lock_error). Could be a
separate, earlier patch (or a TODO comment in this patch to be
addressed with a later patch) since it's fixing an existing bug.
I will add it to my todo list.
I think passing of errno around is too fragile and that we should
avoid ad-hoc save_errno hacks and implement dedicated return codes to
replace errno.
We should only inspect errno immediately after a syscall has failed.
Hope that helps,
Jonathan
--
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 v11 06/41] refs.c: add an err argument to repack_without_refs
Hi,
Looking at 67b8fcee:
Ronnie Sahlberg wrote:
On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
This is about making errno meaningful when commit_packed_refs returns
an error. Probably its API documentation should say so to make it
obvious when people modify it in the future that they should preserve
that property or audit callers.
67b8fcee doesn't address this. While it's often even better to return
an error message or meaningful error number so the caller doesn't have
to worry about errno at all, I think being explicit in the comment
above a declaration about which functions leave behind a meaningful
errno can make error handling saner.
So I still think that the above would be a good idea.
[...]
[...]
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char
**refnames, int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path(packed-refs), errno,
+ err);
+ return -1;
+ }
unable_to_lock_error(git_path(packed-refs), errno);
Via the new call to unable_to_lock_..., repack_without_refs cares
about errno after a failed call to lock_packed_refs. lock_packed_refs
can only fail in hold_lock_file_for_update. hold_lock_file_for_update
is a thin wrapper around lockfile.c::lock_file. lock_file can error
out because
strlen(path) = max_path_len
[...]
Could be a
separate, earlier patch (or a TODO comment in this patch to be
addressed with a later patch) since it's fixing an existing bug.
I will add it to my todo list.
My worry with that is that it is too easy to forget that there is a
problem at all. That's not *that* bad --- if no one remembers, how
serious of a problem was it, really?
Except that it makes reading code difficult. It's easier to read
some code that prints strerror(errno) in a place that is known to
sometimes have errno==0 if there is a comment
/*
* NEEDSWORK: Tweak lock_packed_refs to either reliably
* set errno to a sane value on error or to propagate
* back error information another way.
*/
perror(...);
Otherwise, the reader has to keep scratching their head, wondering
how did this ever work?.
That's why I suggested adding a TODO comment.
I think passing of errno around is too fragile and that we should
avoid ad-hoc save_errno hacks and implement dedicated return codes to
replace errno.
We should only inspect errno immediately after a syscall has failed.
Sure, agreed in principle. ;-)
Thanks,
Jonathan
--
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 v11 06/41] refs.c: add an err argument to repack_without_refs
Hi,
Comments from http://marc.info/?l=gitm=140079653930751w=2:
Ronnie Sahlberg wrote:
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
unable_to_lock_strbuf could be called unable_to_lock_message (which
would make its behavior more obvious IMHO).
[...]
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
This is about making errno meaningful when commit_packed_refs returns
an error. Probably its API documentation should say so to make it
obvious when people modify it in the future that they should preserve
that property or audit callers.
[...]
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path(packed-refs), errno,
+ err);
+ return -1;
+ }
unable_to_lock_error(git_path(packed-refs), errno);
Via the new call to unable_to_lock_..., repack_without_refs cares
about errno after a failed call to lock_packed_refs. lock_packed_refs
can only fail in hold_lock_file_for_update. hold_lock_file_for_update
is a thin wrapper around lockfile.c::lock_file. lock_file can error
out because
strlen(path) = max_path_len
adjust_shared_perm failed [calls error(), clobbers errno]
open failed
So lock_file needs a similar kind of fix, and it's probably worth
updating API documentation for these calls to make it clear that their
errno is used (though that's not a new problem since
repack_without_refs already called unable_to_lock_error). Could be a
separate, earlier patch (or a TODO comment in this patch to be
addressed with a later patch) since it's fixing an existing bug.
Hope that helps,
Jonathan
--
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
