[PATCH 1/2] xopen: explicitly report creation failures

[PATCH 1/2] xopen: explicitly report creation failures

[René Scharfe]

If the flags O_CREAT and O_EXCL are both given then open(2) is supposed
to create the file and error out if it already exists.  The error
message in that case looks like this:

	fatal: could not open 'foo' for writing: File exists

Without further context this is confusing: Why should the existence of
the file pose a problem?  Isn't that a requirement for writing to it?

Add a more specific error message for that case to tell the user that we
actually don't expect the file to preexist, so the example becomes:

	fatal: unable to create 'foo': File exists

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 wrapper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/wrapper.c b/wrapper.c
index 563ad590df..7c6586af32 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
 		if (errno == EINTR)
 			continue;

-		if ((oflag & O_RDWR) == O_RDWR)
+		if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
+			die_errno(_("unable to create '%s'"), path);
+		else if ((oflag & O_RDWR) == O_RDWR)
 			die_errno(_("could not open '%s' for reading and writing"), path);
 		else if ((oflag & O_WRONLY) == O_WRONLY)
 			die_errno(_("could not open '%s' for writing"), path);
--
2.33.0

[PATCH 2/2] use xopen() to handle fatal open(2) failures

[René Scharfe]

Add and apply a semantic patch for using xopen() instead of calling
open(2) and die() or die_errno() explicitly.  This makes the error
messages more consistent and shortens the code.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/add.c                  |  4 +---
 builtin/archive.c              |  4 +---
 builtin/bugreport.c            |  5 +----
 builtin/commit-tree.c          |  4 +---
 builtin/hash-object.c          |  4 +---
 builtin/index-pack.c           |  8 ++------
 builtin/mailsplit.c            |  4 +---
 builtin/merge.c                |  4 +---
 builtin/notes.c                |  4 +---
 builtin/tag.c                  |  4 +---
 builtin/update-index.c         |  4 +---
 contrib/coccinelle/xopen.cocci | 16 ++++++++++++++++
 csum-file.c                    |  8 ++------
 pack-write.c                   |  8 ++------
 run-command.c                  |  4 +---
 15 files changed, 33 insertions(+), 52 deletions(-)
 create mode 100644 contrib/coccinelle/xopen.cocci

diff --git a/builtin/add.c b/builtin/add.c
index 17528e8f92..8ec5ad9f26 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -319,9 +319,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev.diffopt.use_color = 0;
 	rev.diffopt.flags.ignore_dirty_submodules = 1;
-	out = open(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
-	if (out < 0)
-		die(_("Could not open '%s' for writing."), file);
+	out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	rev.diffopt.file = xfdopen(out, "w");
 	rev.diffopt.close_file = 1;
 	if (run_diff_files(&rev, 0))
diff --git a/builtin/archive.c b/builtin/archive.c
index 45d11669aa..7176b041b6 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -12,9 +12,7 @@

 static void create_output_file(const char *output_file)
 {
-	int output_fd = open(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
-	if (output_fd < 0)
-		die_errno(_("could not create archive file '%s'"), output_file);
+	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	if (output_fd != 1) {
 		if (dup2(output_fd, 1) < 0)
 			die_errno(_("could not redirect output"));
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9915a5841d..06ed10dc92 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -171,10 +171,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	get_populated_hooks(&buffer, !startup_info->have_repository);

 	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
-	report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
-
-	if (report < 0)
-		die(_("couldn't create a new file at '%s'"), report_path.buf);
+	report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);

 	if (write_in_full(report, buffer.buf, buffer.len) < 0)
 		die_errno(_("unable to write to %s"), report_path.buf);
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 1031b9a491..63ea322933 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -88,9 +88,7 @@ static int parse_file_arg_callback(const struct option *opt,
 	if (!strcmp(arg, "-"))
 		fd = 0;
 	else {
-		fd = open(arg, O_RDONLY);
-		if (fd < 0)
-			die_errno(_("git commit-tree: failed to open '%s'"), arg);
+		fd = xopen(arg, O_RDONLY);
 	}
 	if (strbuf_read(buf, fd, 0) < 0)
 		die_errno(_("git commit-tree: failed to read '%s'"), arg);
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 640ef4ded5..2e6e2ddd0c 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -53,9 +53,7 @@ static void hash_object(const char *path, const char *type, const char *vpath,
 			unsigned flags, int literally)
 {
 	int fd;
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		die_errno("Cannot open '%s'", path);
+	fd = xopen(path, O_RDONLY);
 	hash_fd(fd, type, vpath, flags, literally);
 }

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865..6cc4890217 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -338,15 +338,11 @@ static const char *open_pack_file(const char *pack_name)
 						"pack/tmp_pack_XXXXXX");
 			pack_name = strbuf_detach(&tmp_file, NULL);
 		} else {
-			output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
-			if (output_fd < 0)
-				die_errno(_("unable to create '%s'"), pack_name);
+			output_fd = xopen(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
 		}
 		nothread_data.pack_fd = output_fd;
 	} else {
-		input_fd = open(pack_name, O_RDONLY);
-		if (input_fd < 0)
-			die_errno(_("cannot open packfile '%s'"), pack_name);
+		input_fd = xopen(pack_name, O_RDONLY);
 		output_fd = -1;
 		nothread_data.pack_fd = input_fd;
 	}
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 664400b816..7baef30569 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -75,9 +75,7 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 		fprintf(stderr, "corrupt mailbox\n");
 		exit(1);
 	}
-	fd = open(name, O_WRONLY | O_CREAT | O_EXCL, 0666);
-	if (fd < 0)
-		die_errno("cannot open output file '%s'", name);
+	fd = xopen(name, O_WRONLY | O_CREAT | O_EXCL, 0666);
 	output = xfdopen(fd, "w");

 	/* Copy it out, while searching for a line that begins with
diff --git a/builtin/merge.c b/builtin/merge.c
index 22f23990b3..47614d8070 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1136,9 +1136,7 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge
 		merge_names = &fetch_head_file;

 	filename = git_path_fetch_head(the_repository);
-	fd = open(filename, O_RDONLY);
-	if (fd < 0)
-		die_errno(_("could not open '%s' for reading"), filename);
+	fd = xopen(filename, O_RDONLY);

 	if (strbuf_read(merge_names, fd, 0) < 0)
 		die_errno(_("could not read '%s'"), filename);
diff --git a/builtin/notes.c b/builtin/notes.c
index 74bba39ca8..71c59583a1 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -172,9 +172,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *

 		/* write the template message before editing: */
 		d->edit_path = git_pathdup("NOTES_EDITMSG");
-		fd = open(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-		if (fd < 0)
-			die_errno(_("could not create file '%s'"), d->edit_path);
+		fd = xopen(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);

 		if (d->given)
 			write_or_die(fd, d->buf.buf, d->buf.len);
diff --git a/builtin/tag.c b/builtin/tag.c
index 452558ec95..065b6bf093 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -293,9 +293,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,

 		/* write the template message before editing: */
 		path = git_pathdup("TAG_EDITMSG");
-		fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-		if (fd < 0)
-			die_errno(_("could not create file '%s'"), path);
+		fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);

 		if (opt->message_given) {
 			write_or_die(fd, buf->buf, buf->len);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index f1f16f2de5..187203e8bb 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -95,9 +95,7 @@ static int create_file(const char *path)
 {
 	int fd;
 	path = get_mtime_path(path);
-	fd = open(path, O_CREAT | O_RDWR, 0644);
-	if (fd < 0)
-		die_errno(_("failed to create file %s"), path);
+	fd = xopen(path, O_CREAT | O_RDWR, 0644);
 	return fd;
 }

diff --git a/contrib/coccinelle/xopen.cocci b/contrib/coccinelle/xopen.cocci
new file mode 100644
index 0000000000..814d7b8a1a
--- /dev/null
+++ b/contrib/coccinelle/xopen.cocci
@@ -0,0 +1,16 @@
+@@
+identifier fd;
+identifier die_fn =~ "^(die|die_errno)$";
+@@
+(
+  fd =
+- open
++ xopen
+  (...);
+|
+  int fd =
+- open
++ xopen
+  (...);
+)
+- if ( \( fd < 0 \| fd == -1 \) ) { die_fn(...); }
diff --git a/csum-file.c b/csum-file.c
index c951cf8277..26e8a6df44 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -131,12 +131,8 @@ struct hashfile *hashfd_check(const char *name)
 	int sink, check;
 	struct hashfile *f;

-	sink = open("/dev/null", O_WRONLY);
-	if (sink < 0)
-		die_errno("unable to open /dev/null");
-	check = open(name, O_RDONLY);
-	if (check < 0)
-		die_errno("unable to open '%s'", name);
+	sink = xopen("/dev/null", O_WRONLY);
+	check = xopen(name, O_RDONLY);
 	f = hashfd(sink, name);
 	f->check_fd = check;
 	f->check_buffer = xmalloc(f->buffer_len);
diff --git a/pack-write.c b/pack-write.c
index f1fc3ecafa..2767b78619 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -75,9 +75,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 			index_name = strbuf_detach(&tmp_file, NULL);
 		} else {
 			unlink(index_name);
-			fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
-			if (fd < 0)
-				die_errno("unable to create '%s'", index_name);
+			fd = xopen(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
 		}
 		f = hashfd(fd, index_name);
 	}
@@ -256,9 +254,7 @@ const char *write_rev_file_order(const char *rev_name,
 			rev_name = strbuf_detach(&tmp_file, NULL);
 		} else {
 			unlink(rev_name);
-			fd = open(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
-			if (fd < 0)
-				die_errno("unable to create '%s'", rev_name);
+			fd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
 		}
 		f = hashfd(fd, rev_name);
 	} else if (flags & WRITE_REV_VERIFY) {
diff --git a/run-command.c b/run-command.c
index f72e72cce7..2961f7e55e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -761,9 +761,7 @@ int start_command(struct child_process *cmd)
 		notify_pipe[0] = notify_pipe[1] = -1;

 	if (cmd->no_stdin || cmd->no_stdout || cmd->no_stderr) {
-		null_fd = open("/dev/null", O_RDWR | O_CLOEXEC);
-		if (null_fd < 0)
-			die_errno(_("open /dev/null failed"));
+		null_fd = xopen("/dev/null", O_RDWR | O_CLOEXEC);
 		set_cloexec(null_fd);
 	}

--
2.33.0

Re: [PATCH 1/2] xopen: explicitly report creation failures

[Carlo Arenas]

On Wed, Aug 25, 2021 at 2:11 PM René Scharfe <l.s.r@web.de> wrote:
>
> diff --git a/wrapper.c b/wrapper.c
> index 563ad590df..7c6586af32 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
>                 if (errno == EINTR)
>                         continue;
>
> -               if ((oflag & O_RDWR) == O_RDWR)
> +               if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
> +                       die_errno(_("unable to create '%s'"), path);

probably over conservative, but && errno == EEXIST?

> +               else if ((oflag & O_RDWR) == O_RDWR)
>                         die_errno(_("could not open '%s' for reading and writing"), path);
>                 else if ((oflag & O_WRONLY) == O_WRONLY)
>                         die_errno(_("could not open '%s' for writing"), path);

Since you are already changing this code, why not take the opportunity
to refactor it
and remove the " == FLAG" part of these conditionals which is
otherwise redundant?

Either way "Reviewed-by", and indeed a nice cleanup.

Carlo

Re: [PATCH 1/2] xopen: explicitly report creation failures

[René Scharfe]

Am 26.08.21 um 01:46 schrieb Carlo Arenas:
> On Wed, Aug 25, 2021 at 2:11 PM René Scharfe <l.s.r@web.de> wrote:
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index 563ad590df..7c6586af32 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
>>                 if (errno == EINTR)
>>                         continue;
>>
>> -               if ((oflag & O_RDWR) == O_RDWR)
>> +               if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
>> +                       die_errno(_("unable to create '%s'"), path);
>
> probably over conservative, but && errno == EEXIST?

No matter what error we got, if O_CREAT and O_EXCL were both given then
we tried to create a file, so this message applies.

>
>> +               else if ((oflag & O_RDWR) == O_RDWR)
>>                         die_errno(_("could not open '%s' for reading and writing"), path);
>>                 else if ((oflag & O_WRONLY) == O_WRONLY)
>>                         die_errno(_("could not open '%s' for writing"), path);
>
> Since you are already changing this code, why not take the opportunity
> to refactor it
> and remove the " == FLAG" part of these conditionals which is
> otherwise redundant?

The repetition is unsightly, but it's a different issue that should be
addressed separately.  Simply removing the comparison feels iffy,
though.  POSIX doesn't seem to forbid e.g. O_RDONLY to be 1, O_WRONLY
to be 2 and O_RDWR to be 3, and then you need to check all masked bits.
I can't think of simpler alternative to the comparison.

> Either way "Reviewed-by", and indeed a nice cleanup.

Thank you!

René

Re: [PATCH 1/2] xopen: explicitly report creation failures

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 26.08.21 um 01:46 schrieb Carlo Arenas:
>> On Wed, Aug 25, 2021 at 2:11 PM René Scharfe <l.s.r@web.de> wrote:
>>>
>>> diff --git a/wrapper.c b/wrapper.c
>>> index 563ad590df..7c6586af32 100644
>>> --- a/wrapper.c
>>> +++ b/wrapper.c
>>> @@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
>>>                 if (errno == EINTR)
>>>                         continue;
>>>
>>> -               if ((oflag & O_RDWR) == O_RDWR)
>>> +               if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
>>> +                       die_errno(_("unable to create '%s'"), path);
>>
>> probably over conservative, but && errno == EEXIST?
>
> No matter what error we got, if O_CREAT and O_EXCL were both given then
> we tried to create a file, so this message applies.

100% agreed.

>>> +               else if ((oflag & O_RDWR) == O_RDWR)
>>>                         die_errno(_("could not open '%s' for reading and writing"), path);
>>>                 else if ((oflag & O_WRONLY) == O_WRONLY)
>>>                         die_errno(_("could not open '%s' for writing"), path);
>>
>> Since you are already changing this code, why not take the opportunity
>> to refactor it
>> and remove the " == FLAG" part of these conditionals which is
>> otherwise redundant?
>
> The repetition is unsightly, but it's a different issue that should be
> addressed separately.  Simply removing the comparison feels iffy,
> though.  POSIX doesn't seem to forbid e.g. O_RDONLY to be 1, O_WRONLY
> to be 2 and O_RDWR to be 3, and then you need to check all masked bits.
> I can't think of simpler alternative to the comparison.

I fully agree that such a change, if done, must be done in an
unrelated patch.  

It is funny that the code is already prepared for such a case where
RDWR is defined as RDONLY|WRONLY.  I wonder if we wrote the series
of comparisons in this order on purpose, or we were just lucky, when
we did 3ff53df7 (wrapper: implement xopen(), 2015-08-04) ;-)


>
>> Either way "Reviewed-by", and indeed a nice cleanup.
>
> Thank you!

Yes, indeed, this is nicely done.