Fix #1762 #1861
Fix #1762 #1861
Conversation
src/libutil/serialise.cc
Outdated
| @@ -67,7 +67,8 @@ void FdSink::write(const unsigned char * data, size_t len) | |||
| try { | |||
| writeFull(fd, data, len); | |||
| } catch (SysError & e) { | |||
| _good = true; | |||
| _good = false; | |||
| throw e; | |||
There was a problem hiding this comment.
Sure, if that's equivalent or better. I'm a bit fuzzy on C++'s exception semantics.
There was a problem hiding this comment.
I am pretty sure it's generally preferred, but I'm similarly reluctant to pose as a C++ semantics expert :). As I understand it, throw e will possibly (maybe always?) create a copy of the exception object which is usually unnecessary and can cause slicing if the object was a derived class from the caught type. I'm not sure in this case there is a problem but I just remember to just use "throw;" whenever possible :).
nix-store --export, nix-store --dump, and nix dump-path would previously
fail silently if writing the data out failed, because
a) FdSink::write ignored exceptions, and
b) the commands relied on FdSink's destructor, which ignores
exceptions, to flush the data out.
This could cause rather opaque issues with installing nixos, because
nix-store --export would happily proceed even if it couldn't write its
data out (e.g. if nix-store --import on the other side of the pipe
failed).
This commit adds tests that expose these issues in the nix-store
commands, and fixes them for all three.
lheckemann
force-pushed
the
write-failure-fixes
branch
from
7086ce2
to
78ac3eb
Compare
|
It seems it should be an error to destruct an FdSink without first calling flush(), then? Not to burden this PR with "fix all the things" but just for sake of discussion/overall code goodness:
Towards the latter, perhaps FdSink should grow a "finish" like the compression sinks have, which would help differentiate flush vs what's essentially explicitly performing what's currently done in the destructors (minus ignoring exceptions). All that said, this LGTM! |
|
I checked for other uses of FdSink and they all seem to call flush as appropriate. |
nix-store --export, nix-store --dump, and nix dump-path would previously
fail silently if writing the data out failed, because
a) FdSink::write ignored exceptions, and
b) the commands relied on FdSink's destructor, which ignores
exceptions, to flush the data out.
This could cause rather opaque issues with installing nixos, because
nix-store --export would happily proceed even if it couldn't write its
data out (e.g. if nix-store --import on the other side of the pipe
failed).
This commit adds tests that expose these issues in the nix-store
commands, and fixes them for all three.