threads / patch / 11563

patchbundle, fast-import: detect write failure

Subject: [PATCH] bundle, fast-import: detect write failure

## tl;dr

13 messages between Jan 10, 2008 and Jan 11, 2008. Diffs are folded; open one to read it.

replies: 12people: 7as markdown or json

Jim Meyering· Jan 10, 2008, 08:54 UTC · lore
I noticed some unchecked writes.  This fixes them.
* bundle.c (create_bundle): Die upon write failure.
* fast-import.c (keep_pack): Die upon write or close failure.
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 bundle.c      |    6 +++---
 fast-import.c |    5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)
Show changes to 2 files +6 −6

bundle.c, fast-import.c

diff --git a/bundle.c b/bundle.c
index be204d8..316aa74 100644
--- a/bundle.c
+++ b/bundle.c
@@ -320,9 +320,9 @@ int create_bundle(struct bundle_header *header, const char *path,
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object *object = revs.pending.objects[i].item;
 		if (object->flags & UNINTERESTING)
-			write(rls.in, "^", 1);
-		write(rls.in, sha1_to_hex(object->sha1), 40);
-		write(rls.in, "\n", 1);
+			write_or_die(rls.in, "^", 1);
+		write_or_die(rls.in, sha1_to_hex(object->sha1), 40);
+		write_or_die(rls.in, "\n", 1);
 	}
 	if (finish_command(&rls))
 		return error ("pack-objects died");
diff --git a/fast-import.c b/fast-import.c
index 74597c9..82e9161 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -878,8 +878,9 @@ static char *keep_pack(char *curr_index_name)
 	keep_fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 	if (keep_fd < 0)
 		die("cannot create keep file");
-	write(keep_fd, keep_msg, strlen(keep_msg));
-	close(keep_fd);
+	write_or_die(keep_fd, keep_msg, strlen(keep_msg));
+	if (close(keep_fd))
+		die("failed to write keep file");

 	snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
 		 get_object_directory(), sha1_to_hex(pack_data->sha1));
--
1.5.4.rc2.85.g71fd
Pierre Habouzit· Jan 10, 2008, 09:17 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

On Thu, Jan 10, 2008 at 08:54:25AM +0000, Jim Meyering wrote:
> 
> I noticed some unchecked writes.  This fixes them.
  Yeah, while we're at it, compiling git with -D_FORTIFY_SOURCE=2 isn't
really brilliant right now, there are quite many places with unchecked
writes, fwrites and chdirs.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Jan 10, 2008, 12:05 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

Hi,
On Thu, 10 Jan 2008, Jim Meyering wrote:
> I noticed some unchecked writes.  This fixes them.
Thank you.
However, you also have this:
> -	close(keep_fd);
> +	if (close(keep_fd))
> +		die("failed to write keep file");

I recently read an article which got me thinking about close(). The author maintained that many mistakes are done by being overzealously defensive; die()ing in case of a close() failure (when open() succeeded!) might be just wrong.

Ciao, Dscho

Jim Meyering· Jan 10, 2008, 12:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle, fast-import: detect write failure

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 5 quoted lines
> However, you also have this:
>
>> -	close(keep_fd);
>> +	if (close(keep_fd))
>> +		die("failed to write keep file");
Yes.  I mentioned that in the commit log:
    * bundle.c (create_bundle): Die upon write failure.
    * fast-import.c (keep_pack): Die upon write or close failure.

But even the summary is accurate if you interpret "write" not as the syscall, but as the semantic push-data-through-OS-to-disk operation.

> I recently read an article which got me thinking about close().  The
> author maintained that many mistakes are done by being overzealously
> defensive; die()ing in case of a close() failure (when open() succeeded!)
> might be just wrong.

No. Whether open succeeded is a separate matter. Avoiding an unreported write (or close-writable-fd) failure is not being "overzealously defensive."

>From "man 2 close",
    -------------
    NOTES
       Not  checking  the return value of close() is a common but nevertheless
       serious programming error.  It is quite possible that errors on a  pre-
       vious  write(2) operation are first reported at the final close().  Not
       checking the return value when closing the file may lead to silent loss
       of data.  This can especially be observed with NFS and with disk quota.
    -------------
Johannes Schindelin· Jan 10, 2008, 12:37 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

Hi,
On Thu, 10 Jan 2008, Jim Meyering wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > I recently read an article which got me thinking about close().  The 
> > author maintained that many mistakes are done by being overzealously 
> > defensive; die()ing in case of a close() failure (when open() 
> > succeeded!) might be just wrong.
> 
> No.  Whether open succeeded is a separate matter. Avoiding an unreported 
> write (or close-writable-fd) failure is not being "overzealously 
> defensive."

Are you aware what this code does? It writes a ".keep" file. Whose purpose is to _exist_, and whose purpose is fulfilled, even if the write or the push-back did not succeed.

I could not care less what the manual says. What is important is if the defensive programming is done mindlessly, and therefore can fail so not gracefully.

Ciao, Dscho

Jim Meyering· Jan 10, 2008, 13:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle, fast-import: detect write failure

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Are you aware what this code does?  It writes a ".keep" file.  Whose
> purpose is to _exist_, and whose purpose is fulfilled, even if the write
> or the push-back did not succeed.
Hi,
I do see what you mean.

If the write is not necessary, then perhaps you would prefer a comment documenting that failures of the write and following close are ignorable. And add a '(void)' stmt prefix, to tell compilers that ignoring the return value is deliberate.

However, even if it's not technically required to fail at that point, if it were my choice, I'd prefer to know when a .keep file whose contents are unimportant just happens to reside on a bad spot on my disk. I/O errors should never be ignored.

> I could not care less what the manual says.  What is important is if the
> defensive programming is done mindlessly, and therefore can fail so not
> gracefully.

On the other hand, if that write failure is truly ignorable, a mindless minimalist :-) might argue that it's best just to omit the syscall.

Pierre Habouzit· Jan 10, 2008, 16:25 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Are you aware what this code does?  It writes a ".keep" file.  Whose
> > purpose is to _exist_, and whose purpose is fulfilled, even if the write
> > or the push-back did not succeed.
> 
> Hi,
> 
> I do see what you mean.
> 
> If the write is not necessary, then perhaps you would prefer a comment
> documenting that failures of the write and following close are ignorable.
> And add a '(void)' stmt prefix, to tell compilers that ignoring the
> return value is deliberate.
  Note that (void) isn't enough with the most recent gcc flavours, which
is a pain. I do use:
#define IGNORE(expr)  do { if (expr) (void)0; } while (0)

for that purpose in my code. I know IGNORE isn't a brilliant name, but it's modeled after the ocaml function doing the same thing.

> However, even if it's not technically required to fail at that point,
> if it were my choice, I'd prefer to know when a .keep file whose
> contents are unimportant just happens to reside on a bad spot on my
> disk.  I/O errors should never be ignored.
  Actually I think .keep files are empty, so the write() should not be
there in the first place, and we should only check for close() right ?
not that it matters that much.
Show 7 quoted lines
> > I could not care less what the manual says.  What is important is if the
> > defensive programming is done mindlessly, and therefore can fail so not
> > gracefully.
> 
> On the other hand, if that write failure is truly ignorable,
> a mindless minimalist :-) might argue that it's best just to
> omit the syscall.
  And leak a file descriptor :)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Jim Meyering· Jan 10, 2008, 18:05 UTC · re: Pierre Habouzit · lore

Re: [PATCH] bundle, fast-import: detect write failure

Pierre Habouzit <madcoder@artemis.madism.org> wrote: ...

Show 5 quoted lines
>> On the other hand, if that write failure is truly ignorable,
>> a mindless minimalist :-) might argue that it's best just to
>> omit the syscall.
>
>   And leak a file descriptor :)

Not that mindless. The *write* syscall, not the close. I would never suggest eliminating the close.

Pierre Habouzit· Jan 10, 2008, 18:18 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

On Thu, Jan 10, 2008 at 06:05:56PM +0000, Jim Meyering wrote:
Show 11 quoted lines
> Pierre Habouzit <madcoder@artemis.madism.org> wrote:
> ....
> >> On the other hand, if that write failure is truly ignorable,
> >> a mindless minimalist :-) might argue that it's best just to
> >> omit the syscall.
> >
> >   And leak a file descriptor :)
> 
> Not that mindless.
> The *write* syscall, not the close.
> I would never suggest eliminating the close.
  oh *oops*
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jakub Narebski· Jan 11, 2008, 09:14 UTC · re: Pierre Habouzit · lore

Re: [PATCH] bundle, fast-import: detect write failure

Pierre Habouzit wrote:
> On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:
Show 8 quoted lines
>> However, even if it's not technically required to fail at that point,
>> if it were my choice, I'd prefer to know when a .keep file whose
>> contents are unimportant just happens to reside on a bad spot on my
>> disk.  I/O errors should never be ignored.
> 
>   Actually I think .keep files are empty, so the write() should not be
> there in the first place, and we should only check for close() right ?
> not that it matters that much.

In theory the .keep file should contain description _why_ the pack is made kept. In practice git creates IIRC empty .kep files.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Jan 11, 2008, 07:36 UTC · re: Jim Meyering · lore

Re: [PATCH] bundle, fast-import: detect write failure

Jim Meyering <jim@meyering.net> writes:
> On the other hand, if that write failure is truly ignorable,
> a mindless minimalist :-) might argue that it's best just to
> omit the syscall.

Usually the contents of .keep file is a small one-line comment that describes who decided that the pack needs to be kept and why, so the answer is no.

In this case, a failure while closing that small .keep file is highly unlikely, and if we ever mange to trigger such a highly unlikely failure, I think we would rather want to *know* about it, as it is likely there is something more seriously wrong going on.

So let's keep that check on close().
David Tweed· Jan 11, 2008, 09:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] bundle, fast-import: detect write failure

On Jan 11, 2008 7:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> In this case, a failure while closing that small .keep file is
> highly unlikely, and if we ever mange to trigger such a highly
> unlikely failure, I think we would rather want to *know* about
> it, as it is likely there is something more seriously wrong
> going on.

On a slightly related note: I've got a patch that handles the issue that I reported a couple of months back that tmp pack/index objects where a write fails partway through are not deleted by any git processing, ie, when for example during git gc --prune we get

fatal: sha1 file '/media/usbdiskc/v.git/objects/tmp_pack_QCYYAi' write error (No space left on device) error: failed to run repack

but the tmp_pack_* isn't deleted. I put my patch on the back burner when Junio declared a moratorium on new behaviours until after 1.5.4 gets released, but will post once things open up again.

As it relates to this discussion: one of the awkward things is that the die stuff doesn't leave any programatic indication (ie, not just a message to stderr) that a file is malformed due to a writing failure. Per Nicolas Pitre's suggestion to delete failed tmp_ files during a "git gc --prune", I just delete ALL tmp_ files at that time. This approach seems a bit risky -- can something like a git-svn fetch which generated tmp_ files by a different route be going on at the same time as a git gc? -- but I couldn't think of another way to do it.

-- 
cheers, dave tweed__________________________
david.tweed@gmail.com
Rm 124, School of Systems Engineering, University of Reading.
"we had no idea that when we added templates we were adding a Turing-
complete compile-time language." -- C++ standardisation committee
Johannes Schindelin· Jan 11, 2008, 11:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] bundle, fast-import: detect write failure

Hi,
On Thu, 10 Jan 2008, Junio C Hamano wrote:
Show 6 quoted lines
> In this case, a failure while closing that small .keep file is highly 
> unlikely, and if we ever mange to trigger such a highly unlikely 
> failure, I think we would rather want to *know* about it, as it is 
> likely there is something more seriously wrong going on.
> 
> So let's keep that check on close().

My comment was not about that _check_, but about having this die() instead of just printing out a warning.

If that close fails, strange things are going on, alright. But neither the open() nor the write() failed at that point, so IMO it would be a mistake to error out _here_. If later stages fail also, well, we can die() there, no?

Ciao, Dscho

← back to recent threads