[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH v2] New dumpstream parser to check version number

From: Ramkumar Ramachandra <artagnon_at_gmail.com>
Date: Thu, 19 Aug 2010 21:28:37 +0530

Hi Daniel,

Daniel Shahaf writes:
> > - SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
> > - NULL, NULL, pool));
> > + SVN_ERR(svn_repos_parse_dumpstream3(stream, parser, parse_baton,
> > + NULL, NULL,
> > + SVN_REPOS_DUMPFILE_FORMAT_VERSION,
>
> Wrong macro: when we introduce dumpfile format 4, this will become "4",
> but I think you want it to remain "3" even then, right?

Actually, I'd want to svnrdump tests to fail so someone (or me)
immediately fixes it to use version 4: it'll probably be a performance
boost as well.

> > +#define SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION -1
> > #define SVN_REPOS_DUMPFILE_UUID "UUID"
> > #define SVN_REPOS_DUMPFILE_CONTENT_LENGTH "Content-length"
> >
> > @@ -2646,6 +2647,11 @@ typedef svn_repos_parse_fns2_t svn_repos_parser_fn
> > * @a cancel_baton as argument to see if the client wishes to cancel
> > * the dump.
> > *
> > + * If @a exact_version is SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION,
>
> #define macroname SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION
>
> In doxygen, you can write "#macroname" or "@c macroname". (IIRC the first
> makes a link and the second makes constant-width font; untested.)

Ok, thanks for the pointer. Fixed.

> > + * it is ignored and the dumpstream is parsed without this
> > + * information. Else, the function checks the dumpstream's version
> > + * number, and errors out if there's a mismatch.
> > + *
>
> Sometimes, in situations like this we guarantee exactly which error code
> will be returned, for the benefit of API users who want to trap a
> specific (non-fatal) error condition. See svn_ra_open4() for an
> example.

Oh. Is this necessary though? I'm going to move out this code into a
callback soon- users can handle it there after that.

> > +SVN_DEPRECATED
> > +svn_error_t *svn_repos_parse_dumpstream2(svn_stream_t *stream,
> > + const svn_repos_parse_fns2_t *parse_fns,
> > + void *parse_baton,
>
> Style nitpick:
> The parameters aren't aligned properly. (the previous declaration does
> it correctly.)

Fixed.

> I'm not sure what we gain by committing this patch when you already have
> a parse_fns3_t patch outstanding. (I haven't looked at that thread
> yet.) Wouldn't it make more sense to first commit that patch, than to
> commit this one, then that one, and then a revision of that one to use
> the new API?

That patch is still an RFC, and it's unlikely to be approved soon I
think. If I were able to send a series, it would roughly look like
this:
1. Create parse_dumpstream3 to include the logic for checking equality
   in version.
2. Note the lack of flexibility in 1, and create a new struct (the
   other patch).
3. Modify parse_dumpstream3 to use the new struct, and move out the
   logic for checking the version into a fresh callback.

> Note: it's acceptable to post patches that depend on previous patches.
> (So you could write this patch in terms of parse_fns3_t directly.)
>
> There are a number of ways to manage the interdependencies... (you
> mentioned quilt on IRC; I know some folks here use Mercurial patch
> queues and similar tricks)

Oh, ok. I'll learn to use Quilt then. Using Git to stage is a bit of
an overkill.

-- Ram

-- 8< --
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 986884)
+++ subversion/include/svn_repos.h (working copy)
@@ -2286,6 +2286,7 @@ svn_repos_node_from_baton(void *edit_baton);
 /* The RFC822-style headers in our dumpfile format. */
 #define SVN_REPOS_DUMPFILE_MAGIC_HEADER "SVN-fs-dump-format-version"
 #define SVN_REPOS_DUMPFILE_FORMAT_VERSION 3
+#define SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION -1
 #define SVN_REPOS_DUMPFILE_UUID "UUID"
 #define SVN_REPOS_DUMPFILE_CONTENT_LENGTH "Content-length"
 
@@ -2646,6 +2647,11 @@ typedef svn_repos_parse_fns2_t svn_repos_parser_fn
  * @a cancel_baton as argument to see if the client wishes to cancel
  * the dump.
  *
+ * If @a exact_version is #SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION,
+ * it is ignored and the dumpstream is parsed without this
+ * information. Else, the function checks the dumpstream's version
+ * number, and errors out if there's a mismatch.
+ *
  * This parser has built-in knowledge of the dumpfile format, but only
  * in a general sense:
  *
@@ -2661,16 +2667,31 @@ typedef svn_repos_parse_fns2_t svn_repos_parser_fn
  * This is enough knowledge to make it easy on vtable implementors,
  * but still allow expansion of the format: most headers are ignored.
  *
- * @since New in 1.1.
+ * @since New in 1.7.
  */
 svn_error_t *
-svn_repos_parse_dumpstream2(svn_stream_t *stream,
+svn_repos_parse_dumpstream3(svn_stream_t *stream,
                             const svn_repos_parse_fns2_t *parse_fns,
                             void *parse_baton,
                             svn_cancel_func_t cancel_func,
                             void *cancel_baton,
+ int exact_version,
                             apr_pool_t *pool);
 
+/**
+ * Similar to svn_repos_parse_dumpstream3(), but with @a exact_version
+ * always set to SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION.
+ *
+ * @since New in 1.1.
+ * @deprecated Provided for backward compatibility with the 1.6 API.
+ */
+SVN_DEPRECATED
+svn_error_t *svn_repos_parse_dumpstream2(svn_stream_t *stream,
+ const svn_repos_parse_fns2_t *parse_fns,
+ void *parse_baton,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool);
 
 /**
  * Set @a *parser and @a *parse_baton to a vtable parser which commits new
Index: subversion/libsvn_repos/load.c
===================================================================
--- subversion/libsvn_repos/load.c (revision 986884)
+++ subversion/libsvn_repos/load.c (working copy)
@@ -654,14 +654,29 @@ parse_format_version(const char *versionstring, in
 }
 
 
+svn_error_t *
+svn_repos_parse_dumpstream2(svn_stream_t *stream,
+ const svn_repos_parse_fns2_t *parse_fns,
+ void *parse_baton,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool)
+{
+ return svn_repos_parse_dumpstream3(stream, parse_fns, parse_baton,
+ cancel_func, cancel_baton,
+ SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION,
+ pool);
+}
 
+
 /* The Main Parser Logic */
 svn_error_t *
-svn_repos_parse_dumpstream2(svn_stream_t *stream,
+svn_repos_parse_dumpstream3(svn_stream_t *stream,
                             const svn_repos_parse_fns2_t *parse_fns,
                             void *parse_baton,
                             svn_cancel_func_t cancel_func,
                             void *cancel_baton,
+ int exact_version,
                             apr_pool_t *pool)
 {
   svn_boolean_t eof;
@@ -690,6 +705,15 @@ svn_error_t *
     return svn_error_createf(SVN_ERR_STREAM_MALFORMED_DATA, NULL,
                              _("Unsupported dumpfile version: %d"), version);
 
+ /* Error out if exact_version doesn't match version exactly */
+ /* ### TODO: Extend svn_parse_fns2_t to provide another callback for
+ ### version number; that way we can impose arbitrary constraints
+ ### on it, not just check for exact version */
+ if (exact_version != SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION
+ && exact_version != version)
+ return svn_error_createf(SVN_ERR_STREAM_MALFORMED_DATA, NULL,
+ _("Unsupported dumpfile version: %d"), version);
+
   /* A dumpfile "record" is defined to be a header-block of
      rfc822-style headers, possibly followed by a content-block.
 
Index: subversion/svnrdump/load_editor.c
===================================================================
--- subversion/svnrdump/load_editor.c (revision 986884)
+++ subversion/svnrdump/load_editor.c (working copy)
@@ -540,8 +540,10 @@ drive_dumpstream_loader(svn_stream_t *stream,
   pb = parse_baton;
 
   SVN_ERR(svn_ra_get_repos_root2(session, &(pb->root_url), pool));
- SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
- NULL, NULL, pool));
+ SVN_ERR(svn_repos_parse_dumpstream3(stream, parser, parse_baton,
+ NULL, NULL,
+ SVN_REPOS_DUMPFILE_FORMAT_VERSION,
+ pool));
 
   return SVN_NO_ERROR;
 }
Received on 2010-08-19 18:01:11 CEST

This is an archived mail posted to the Subversion Dev mailing list.