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

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

[Jim Meyering]

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(-)

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

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

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

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

[Jim Meyering]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 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.
    -------------

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

[Johannes Schindelin]

Hi,

On Thu, 10 Jan 2008, Jim Meyering wrote:

> 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

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

[Jim Meyering]

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.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:
> 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.

> > 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Jim Meyering]

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.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Thu, Jan 10, 2008 at 06:05:56PM +0000, Jim Meyering wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Jakub Narebski]

Pierre Habouzit wrote:
> On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:

>> 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

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

[Junio C Hamano]

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().

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

[David Tweed]

On Jan 11, 2008 7:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Johannes Schindelin]

Hi,

On Thu, 10 Jan 2008, Junio C Hamano wrote:

> 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