Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c

Branko Čibej <brane_at_wandisco.com> writes:

> FWIW, I think in this case revving the API without deprecating the old
> one is justified. Another option would be to invent a different API name
> for the flushing stream, e.g., svn_stream_from_aprfile_flushed or some
> such. This way we'd also avoid the dilemma about disown by simply not
> having that flag in the new function.

Are there bits missing from r1680819? svn_fs_fs__pack_revprops_shard()
still uses a stream to write the manifest file and it has not been
modified to flush. write_non_packed_revprop() also uses a stream that
is not flushed.

We could create a "flushing stream" that gets chained:

struct baton_flush_t {
  svn_stream_t *stream;
  apr_pool_t *pool;
};

static svn_error_t *
close_handler_flush(void *baton)
{
  struct baton_flush_t *b = baton;

  SVN_ERR(svn_io_file_flush_to_disk(svn_stream__aprfile(b->stream), b->pool));
  SVN_ERR(svn_stream_close(b->stream));
  return SVN_NO_ERROR;
}

svn_error_t *
svn_stream_flush(svn_stream_t **flush,
                 svn_stream_t *stream,
                 apr_pool_t *pool)
{
  struct baton_flush_t *baton;

  if (!svn_stream__aprfile(stream))
    return svn_error_create("No file to flush");

  baton = apr_palloc(...);
  *flush = svn_stream_create(baton, pool);
  svn_stream_set_close(*flush, close_handler_flush);
  svn_stream_set_read(...);
  ...

  return SVN_NO_ERROR;
}

That would allow the flushing of files we are not going to close which
probably works but may not be something we want. If we want to avoid
that then perhaps we use a more specialised svn_stream_flush() that only
supports streams created by svn_stream_from_aprfile2() by detecting
close_fn==close_hadler_apr.

svn_error_t *
svn_stream_flush(svn_stream_t **flush,
                 svn_stream_t *stream,
                 apr_pool_t *pool)
{
  struct baton_flush_t *baton;

  if (stream->close_fn != close_handler_apr)
    return svn_error_create("No closing file to flush")

  ...
}

Another approach would be to have a function to enable flushing directly
on the stream created by svn_stream_from_aprfile2() without creating a
new stream. This is probably the smallest/simplest patch. After
reverting r1680819:

Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 1680818)
+++ subversion/include/svn_io.h (working copy)
@@ -1087,6 +1087,9 @@ svn_stream_from_aprfile2(apr_file_t *file,
                          svn_boolean_t disown,
                          apr_pool_t *pool);
 
+svn_error_t *
+svn_stream_flush_on_close(svn_stream_t *stream);
+
 /** Similar to svn_stream_from_aprfile2(), except that the file will
  * always be disowned.
  *
Index: subversion/libsvn_fs_fs/revprops.c
===================================================================
--- subversion/libsvn_fs_fs/revprops.c (revision 1680818)
+++ subversion/libsvn_fs_fs/revprops.c (working copy)
@@ -660,6 +660,7 @@ write_non_packed_revprop(const char **final_path,
   SVN_ERR(svn_stream_open_unique(&stream, tmp_path,
                                  svn_dirent_dirname(*final_path, pool),
                                  svn_io_file_del_none, pool, pool));
+ SVN_ERR(svn_stream_flush_on_close(stream));
   SVN_ERR(svn_hash_write2(proplist, stream, SVN_HASH_TERMINATOR, pool));
   SVN_ERR(svn_stream_close(stream));
 
@@ -875,6 +876,7 @@ repack_stream_open(svn_stream_t **stream,
                                                   pool),
                            APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
   *stream = svn_stream_from_aprfile2(file, FALSE, pool);
+ SVN_ERR(svn_stream_flush_on_close(*stream));
 
   return SVN_NO_ERROR;
 }
@@ -1206,6 +1208,7 @@ svn_fs_fs__copy_revprops(const char *pack_file_dir
 
   /* write the pack file content to disk */
   stream = svn_stream_from_aprfile2(pack_file, FALSE, scratch_pool);
+ SVN_ERR(svn_stream_flush_on_close(stream));
   SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
   SVN_ERR(svn_stream_close(stream));
 
@@ -1244,6 +1247,7 @@ svn_fs_fs__pack_revprops_shard(const char *pack_fi
   SVN_ERR(svn_io_dir_make(pack_file_dir, APR_OS_DEFAULT, scratch_pool));
   SVN_ERR(svn_stream_open_writable(&manifest_stream, manifest_file_path,
                                    scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_flush_on_close(manifest_stream));
 
   /* revisions to handle. Special case: revision 0 */
   start_rev = (svn_revnum_t) (shard * max_files_per_dir);
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1680818)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -911,6 +911,29 @@ close_handler_apr(void *baton)
 }
 
 static svn_error_t *
+close_handler_flush(void *baton)
+{
+ struct baton_apr *btn = baton;
+
+ SVN_ERR(svn_io_file_flush_to_disk(btn->file, btn->pool));
+ SVN_ERR(close_handler_apr(baton));
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_stream_flush_on_close(svn_stream_t *stream)
+{
+ if (stream->close_fn != close_handler_apr)
+ return svn_error_create(SVN_ERR_STREAM_NOT_SUPPORTED, NULL,
+ _("No closing file to flush"));
+
+ svn_stream_set_close(stream, close_handler_flush);
+
+ return SVN_NO_ERROR;
+}
+
+
+static svn_error_t *
 mark_handler_apr(void *baton, svn_stream_mark_t **mark, apr_pool_t *pool)
 {
   struct baton_apr *btn = baton;

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*