close failures to stdout not detected
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
| pbzip2 |
Undecided
|
Yavor Nikolov |
Bug Description
Using -c option, with stdout directed to a file on an AFS fileserver, write failures (typically due to quota-full errors) are not detected.
With AFS, some failures are not detected until close(), since writes go into a local cache. A quick perusal of the source suggests pbzip2 doesn't close() stdout.
Related branches
- Yavor Nikolov: Approve on 2014-04-13
-
Diff: 45 lines (+16/-2)2 files modifiedChangeLog (+3/-1)
pbzip2.cpp (+13/-1)
Changed in pbzip2: | |
status: | New → In Progress |
assignee: | nobody → Yavor Nikolov (yavor-nikolov) |
Yavor Nikolov (yavor-nikolov) wrote : | #2 |
@richard-brittain, thanks for reporting this!
I pushed a fix for that here: https:/
Would you be able to test it?
If all is working fine I think we'll be able to release version with that fix soon.
Richard Brittain (richard-brittain) wrote : Re: [Bug 1300876] Re: close failures to stdout not detected | #3 |
On Thu, 10 Apr 2014, Yavor Nikolov wrote:
> @richard-brittain, thanks for reporting this!
I love this program. It has made a huge difference to some of our work -
we generally use it for compressing multi-GB VM disk images.
I'm a bit rusty on coding - it has been a while since I looked at anything
as complex as this.
> I pushed a fix for that here: https:/
> Would you be able to test it?
> If all is working fine I think we'll be able to release version with that fix soon.
Thanks. I'll test this, but skimming your diffs, I think there may be too
many places where you removed the check for stdout vs named file.
Anywhere that we detect an error and are going to abort anyway, it would
be useful to close stout, but the exit fail status is probably already
going to get set. However, when looping over multiple input files which
complete without error, we don't want to close stdout inside the loop - in
this case it effectively concatenates the input files to a single
compressed output file (e.g., the code near line 2078 should retain the
'if (OutputStdOut == 0)' block.)
That's why I suggested an explicit close only for stdout, at the end of
the big 'for' loop, although I suspected there may be other places that
need it too.
We also need to still skip the writeFileMetaData() call on stdout, since
it isn't a real file.
Another very minor error - I think the error message for a close failure
is missing the word 'not'!
"pbzip2: *ERROR: Could close output file! Aborting..."
Richard
--
Richard Brittain, Research Computing Group,
<email address hidden> 6-2085
Yavor Nikolov (yavor-nikolov) wrote : | #4 |
Thanks a lot for your feedback, Richard!
Yes, you're right - I added closing of stdout on too many places.
I'll give it a more careful look to see where exactly it needs to be closed. What matters is to ensure that we check exit code of the final close.
Yavor
Yavor Nikolov (yavor-nikolov) wrote : | #5 |
I updated the code in my (bug-1300876-explicit-
Changed in pbzip2: | |
status: | In Progress → Fix Committed |
milestone: | none → 1.1.9 |
Changed in pbzip2: | |
status: | Fix Committed → Fix Released |
This may affect NFS as well as AFS.
Suggested patch:
--- pbzip2.cpp 2014-04-10 03:01:54.000000001 +0000 ------- ------- ------- ------- ------- --\n");
+++ pbzip2.cpp-new 2014-04-10 02:50:09.000000001 +0000
@@ -4706,6 +4706,18 @@
fprintf(stderr, "------
} /* for */
+ // Explicit close on stdout if we've been writing there, after all input has been processed FILENO) ; :getInstance( )->saveError( ); error(EF_ EXIT, 1, "pbzip2: *ERROR: Failed to close output file! Aborting...\n");
+ if (OutputStdOut == 1)
+ {
+ ret = close(STDOUT_
+ if (ret == -1)
+ {
+ ErrorContext:
+ handle_
+ exit(1);
+ }
+ }
+