Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees
Hi Stefan,
On Tue, 24 Apr 2018, Stefan Beller wrote:
> On Tue, Apr 24, 2018 at 11:51 AM, Johannes Schindelin
> wrote:
>
> >
> > Oy vey. How many more mistakes can I introduce in one commit...
> >
>
> I ask this myself all the time, but Software is hard when not having
> computer assisted checks.
Right. I still hope to find some time to play with Infer (Open Source
alternative to Coverity, started at Facebook). Windows support is not
there yet, which is the main hold-up for me (if I had the time, I would
spend it on adding Windows support to Infer). I could imagine that Infer
might be flexible enough to ask these questions programmatically:
- is there a code path that forgets to close() file handles?
- is there a code path that forgets to release strbufs?
- are there redundant strbuf_release() calls?
> The test suite doesn't quite count here, as it doesn't yell loudly
> enough for leaks in corner cases.
Right, and running everything through valgrind is not fast enough.
Besides, this misses non-Linux code paths.
> Thanks for taking these seriously, I was unsure if the
> first issues (close() clobbering the errno) were sever enough
> to bother. It complicates the code, but the effect is theoretical)
> (for EBADF) or a real niche corner case (EINTR).
Maybe it might not be *that* serious currently. It is incorrect, though,
and makes the code less reusable. I like to copy-edit my code a lot when
refactoring is not an option.
> Speaking of that, I wonder if we eventually want to have
> a wrapper
>
> int xclose(int fd)
> {
> int err = errno;
> int ret = close(fd)
> if (errno == EINTR)
> /* on linux we don't care about this, other OSes? */
> ;
> errno = err;
> return ret;
> }
>
> Though not in this series.
Or maybe a Coccinelle rule? (Still not in this series, though.)
Ciao,
Dscho
Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees
Hi Johannes,
On Tue, Apr 24, 2018 at 11:51 AM, Johannes Schindelin
wrote:
>
> Oy vey. How many more mistakes can I introduce in one commit...
>
I ask this myself all the time, but Software is hard when not having
computer assisted checks. The test suite doesn't quite count here,
as it doesn't yell loudly enough for leaks in corner cases.
Thanks for taking these seriously, I was unsure if the
first issues (close() clobbering the errno) were sever enough
to bother. It complicates the code, but the effect is theoretical)
(for EBADF) or a real niche corner case (EINTR).
Speaking of that, I wonder if we eventually want to have
a wrapper
int xclose(int fd)
{
int err = errno;
int ret = close(fd)
if (errno == EINTR)
/* on linux we don't care about this, other OSes? */
;
errno = err;
return ret;
}
Though not in this series.
Thanks,
Stefan
Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees
Hi Stefan,
On Mon, 23 Apr 2018, Stefan Beller wrote:
> On Fri, Apr 20, 2018 at 3:21 PM, Johannes Schindelin
> wrote:
> > @@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum
> > object_type type,
> > - if (strbuf_read(&result, cmd.out, 41) < 0)
> > - die_errno("unable to read from mktree");
> > + if (strbuf_read(&result, cmd.out, 41) < 0) {
> > + close(fd);
> > + close(cmd.out);
> > + return error_errno("unable to read from mktree");
>
> So before the errno is coming directly from strbuf_read,
> which will set errno on error to the desired errno.
> (It will come from an underlying read())
Yes, you are right!
> However close() may fail and clobber errno,
> so I would think we'd need to
>
> if (strbuf_read(&result, cmd.out, 41) < 0) {
> int err = errno; /* close shall not clobber errno */
> close(fd);
> close(cmd.out);
> errno = err;
> return error_errno(...);
> }
I went for the easier route: call error_errno() before close(fd), and then
return -1 after close(cmd.out). Since error_errno() always returns -1, the
result is pretty much the same (I do not think that we want the caller of
import_object() to rely on the errno).
> > - if (fstat(fd, &st) < 0)
> > - die_errno("unable to fstat %s", filename);
> > + if (fstat(fd, &st) < 0) {
> > + close(fd);
> > + return error_errno("unable to fstat %s", filename);
> > + }
>
> Same here?
Yep.
> An alternative would be to do
> ret = error_errno(...)
> close (..)
> return ret;
I even saved one variable ;-)
> > @@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref,
> > int force, int raw)
> > struct strbuf ref = STRBUF_INIT;
> >
> > if (get_oid(object_ref, &old_oid) < 0)
> > - die("Not a valid object name: '%s'", object_ref);
> > + return error("Not a valid object name: '%s'", object_ref);
> >
> > type = oid_object_info(&old_oid, NULL);
> > if (type < 0)
> > - die("unable to get object type for %s",
> > oid_to_hex(&old_oid));
> > + return error("unable to get object type for %s",
> > +oid_to_hex(&old_oid));
> >
> > - check_ref_valid(&old_oid, &prev, &ref, force);
> > + if (check_ref_valid(&old_oid, &prev, &ref, force))
> > + return -1;
> > strbuf_release(&ref);
> >
> > - export_object(&old_oid, type, raw, tmpfile);
> > + if (export_object(&old_oid, type, raw, tmpfile))
> > + return -1;
> > if (launch_editor(tmpfile, NULL, NULL) < 0)
> > - die("editing object file failed");
> > - import_object(&new_oid, type, raw, tmpfile);
> > + return error("editing object file failed");
> > + if (import_object(&new_oid, type, raw, tmpfile))
> > + return -1;
> >
> > free(tmpfile);
>
> Do we need to free tmpfile in previous returns?
Oy vey. How many more mistakes can I introduce in one commit...
> > @@ -394,24 +422,29 @@ static int create_graft(int argc, const char **argv,
> > int force)
> > unsigned long size;
> >
> > if (get_oid(old_ref, &old_oid) < 0)
> > - die(_("Not a valid object name: '%s'"), old_ref);
> > - commit = lookup_commit_or_die(&old_oid, old_ref);
> > + return error(_("Not a valid object name: '%s'"), old_ref);
> > + commit = lookup_commit_reference(&old_oid);
> > + if (!commit)
> > + return error(_("could not parse %s"), old_ref);
> >
> > buffer = get_commit_buffer(commit, &size);
> > strbuf_add(&buf, buffer, size);
> > unuse_commit_buffer(commit, buffer);
> >
> > - replace_parents(&buf, argc - 1, &argv[1]);
> > + if (replace_parents(&buf, argc - 1, &argv[1]) < 0)
> > + return -1;
> >
> > if (remove_signature(&buf)) {
> > warning(_("the original commit '%s' has a gpg signature."),
> > old_ref);
> > warning(_("the signature will be removed in the replacement
> > commit!"));
> > }
> >
> > - check_mergetags(commit, argc, argv);
> > + if (check_mergetags(commit, argc, argv))
> > + return -1;
> >
> > if (write_object_file(buf.buf, buf.len, commit_type, &new_oid))
> > - die(_("could not write replacement commit for: '%s'"),
> > old_ref);
> > + return error(_("could not write replacement commit for:
> > '%s'"),
> > +old_ref);
> >
> > strbuf_release(&buf);
>
> Release &buf in the other return cases, too?
Absolutely.
Thank you for helping me improve these patches,
Dscho
Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees
Hi Johannes,
On Fri, Apr 20, 2018 at 3:21 PM, Johannes Schindelin
wrote:
> @@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum
> object_type type,
> - if (strbuf_read(&result, cmd.out, 41) < 0)
> - die_errno("unable to read from mktree");
> + if (strbuf_read(&result, cmd.out, 41) < 0) {
> + close(fd);
> + close(cmd.out);
> + return error_errno("unable to read from mktree");
So before the errno is coming directly from strbuf_read,
which will set errno on error to the desired errno.
(It will come from an underlying read())
However close() may fail and clobber errno,
so I would think we'd need to
if (strbuf_read(&result, cmd.out, 41) < 0) {
int err = errno; /* close shall not clobber errno */
close(fd);
close(cmd.out);
errno = err;
return error_errno(...);
}
> - if (fstat(fd, &st) < 0)
> - die_errno("unable to fstat %s", filename);
> + if (fstat(fd, &st) < 0) {
> + close(fd);
> + return error_errno("unable to fstat %s", filename);
> + }
Same here?
An alternative would be to do
ret = error_errno(...)
close (..)
return ret;
> @@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref, int
> force, int raw)
> struct strbuf ref = STRBUF_INIT;
>
> if (get_oid(object_ref, &old_oid) < 0)
> - die("Not a valid object name: '%s'", object_ref);
> + return error("Not a valid object name: '%s'", object_ref);
>
> type = oid_object_info(&old_oid, NULL);
> if (type < 0)
> - die("unable to get object type for %s", oid_to_hex(&old_oid));
> + return error("unable to get object type for %s",
> +oid_to_hex(&old_oid));
>
> - check_ref_valid(&old_oid, &prev, &ref, force);
> + if (check_ref_valid(&old_oid, &prev, &ref, force))
> + return -1;
> strbuf_release(&ref);
>
> - export_object(&old_oid, type, raw, tmpfile);
> + if (export_object(&old_oid, type, raw, tmpfile))
> + return -1;
> if (launch_editor(tmpfile, NULL, NULL) < 0)
> - die("editing object file failed");
> - import_object(&new_oid, type, raw, tmpfile);
> + return error("editing object file failed");
> + if (import_object(&new_oid, type, raw, tmpfile))
> + return -1;
>
> free(tmpfile);
Do we need to free tmpfile in previous returns?
> @@ -394,24 +422,29 @@ static int create_graft(int argc, const char **argv,
> int force)
> unsigned long size;
>
> if (get_oid(old_ref, &old_oid) < 0)
> - die(_("Not a valid object name: '%s'"), old_ref);
> - commit = lookup_commit_or_die(&old_oid, old_ref);
> + return error(_("Not a valid object name: '%s'"), old_ref);
> + commit = lookup_commit_reference(&old_oid);
> + if (!commit)
> + return error(_("could not parse %s"), old_ref);
>
> buffer = get_commit_buffer(commit, &size);
> strbuf_add(&buf, buffer, size);
> unuse_commit_buffer(commit, buffer);
>
> - replace_parents(&buf, argc - 1, &argv[1]);
> + if (replace_parents(&buf, argc - 1, &argv[1]) < 0)
> + return -1;
>
> if (remove_signature(&buf)) {
> warning(_("the original commit '%s' has a gpg signature."),
> old_ref);
> warning(_("the signature will be removed in the replacement
> commit!"));
> }
>
> - check_mergetags(commit, argc, argv);
> + if (check_mergetags(commit, argc, argv))
> + return -1;
>
> if (write_object_file(buf.buf, buf.len, commit_type, &new_oid))
> - die(_("could not write replacement commit for: '%s'"),
> old_ref);
> + return error(_("could not write replacement commit for:
> '%s'"),
> +old_ref);
>
> strbuf_release(&buf);
Release &buf in the other return cases, too?
Thanks,
Stefan
[PATCH v4 04/11] replace: "libify" create_graft() and callees
File this away as yet another patch in the "libification" category.
As with all useful functions, in the next commit we want to use
create_graft() from a higher-level function where it would be
inconvenient if the called function simply die()s: if there is a
problem, we want to let the user know how to proceed, and the callee
simply has no way of knowing what to say.
Signed-off-by: Johannes Schindelin
---
builtin/replace.c | 135 --
1 file changed, 84 insertions(+), 51 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index e345a5a0f1c..f63df405fd0 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char
*format)
else if (!strcmp(format, "long"))
data.format = REPLACE_FORMAT_LONG;
else
- die("invalid replace format '%s'\n"
- "valid formats are 'short', 'medium' and 'long'\n",
- format);
+ return error("invalid replace format '%s'\n"
+"valid formats are 'short', 'medium' and 'long'\n",
+format);
for_each_replace_ref(show_reference, (void *)&data);
@@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char
*ref,
return 0;
}
-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
struct object_id *prev,
struct strbuf *ref,
int force)
@@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object,
strbuf_reset(ref);
strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object));
if (check_refname_format(ref->buf, 0))
- die("'%s' is not a valid ref name.", ref->buf);
+ return error("'%s' is not a valid ref name.", ref->buf);
if (read_ref(ref->buf, prev))
oidclr(prev);
else if (!force)
- die("replace ref '%s' already exists", ref->buf);
+ return error("replace ref '%s' already exists", ref->buf);
+ return 0;
}
static int replace_object_oid(const char *object_ref,
@@ -161,28 +162,31 @@ static int replace_object_oid(const char *object_ref,
struct strbuf ref = STRBUF_INIT;
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+ int res = 0;
obj_type = oid_object_info(object, NULL);
repl_type = oid_object_info(repl, NULL);
if (!force && obj_type != repl_type)
- die("Objects must be of the same type.\n"
- "'%s' points to a replaced object of type '%s'\n"
- "while '%s' points to a replacement object of type '%s'.",
- object_ref, type_name(obj_type),
- replace_ref, type_name(repl_type));
+ return error("Objects must be of the same type.\n"
+"'%s' points to a replaced object of type '%s'\n"
+"while '%s' points to a replacement object of "
+"type '%s'.",
+object_ref, type_name(obj_type),
+replace_ref, type_name(repl_type));
- check_ref_valid(object, &prev, &ref, force);
+ if (check_ref_valid(object, &prev, &ref, force))
+ return -1;
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, repl, &prev,
0, NULL, &err) ||
ref_transaction_commit(transaction, &err))
- die("%s", err.buf);
+ res = error("%s", err.buf);
ref_transaction_free(transaction);
strbuf_release(&ref);
- return 0;
+ return res;
}
static int replace_object(const char *object_ref, const char *replace_ref, int
force)
@@ -190,9 +194,11 @@ static int replace_object(const char *object_ref, const
char *replace_ref, int f
struct object_id object, repl;
if (get_oid(object_ref, &object))
- die("Failed to resolve '%s' as a valid ref.", object_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+object_ref);
if (get_oid(replace_ref, &repl))
- die("Failed to resolve '%s' as a valid ref.", replace_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+replace_ref);
return replace_object_oid(object_ref, &object, replace_ref, &repl,
force);
}
@@ -202,7 +208,7 @@ static int replace_object(const char *object_ref, const
char *replace_ref, int f
* If "raw" is true, then the object's raw contents are printed according to
* "type". Otherwise, we pretty-print the contents for human
