Philip Martin <philip.martin_at_wandisco.com> writes:
> Yes, that looks good.
[...]
Julian Foad <julianfoad_at_btopenworld.com> writes:
> Just a couple of comments, from a quick read of the patch.
[...]
Philip, Julian, thank you for giving this patch a look.
As I said in my previous e-mails, I think that the committed fix is a better
choice here, as opposed to attempts to achieve the same behavior using the
stream API. With a couple of fresh thoughts, I can conclude that the approach
with svn_stream_flush() is also a poor solution for this problem:
1) We cannot really use svn_stream_flush(TRUE) + svn_stream_close() with
a compressed stream that is chained over a file stream or a stream that we
expect to be consistent with the underlying device. A compressed stream
writes the final data chunk in close_handler_gz() and calls deflate() with
Z_FINISH. In this case, closing the compressed stream would require a
full flush of the underlying stream in order to guarantee consistency.
Leaving the compressed streams without a flush handler implementation is
also a poor choice because the caller of svn_stream_flush() would have to
know the details about the given stream in order to avoid receiving the
SVN_ERR_STREAM_NOT_SUPPORTED error.
I believe that this is an example where achieving the proper behavior with
files is simple, but abstracting it with streams is a challenge.
2) The same argument applies to any generic stream that writes data in its
close handler. If you have such a stream anywhere in the chain, using
svn_stream_flush(TRUE) before svn_stream_close() wouldn't be enough to
guarantee the consistency of the underlying device.
3) Doing so would make this fix a 1.9.0 release blocker. Furthermore, we
would probably have problems backporting it to 1.8.x, as I think that
backporting this API as svn_stream__flush() would be a destabilizing
change.
FSFS currently uses apr_file_t and svn_stream_t, depending on the context of
the operation, and I find this reasonable. While streams are generally good
for serializing data, there are certain file operations that cannot be properly
abstracted on the stream layer — for example, an offset-based apr_file_seek()
or an ability to retrieve a file name or size.
We can create a stream from a file when we need to serialize or deserialize
data. However, trying to achieve the opposite, i.e., pushing file-specific
operations through several abstraction layers *is* going to introduce different
sorts of problems — API functions that can only work with certain stream types,
stream open functions that keep the knowledge about the necessity of an fsync()
before closing the stream, runtime errors depending on whether the operation is
supported by the particular stream or not, and, possibly, other.
Regards,
Evgeny Kotkov
Received on 2015-05-27 17:35:29 CEST