Philip Martin <philip.mar...@wandisco.com> writes: > Yes, that looks good.
[...] Julian Foad <julianf...@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
