On 9/30/05, Max Bowsher <maxb@ukf.net> wrote:
> Daniel L. Rall wrote:
> > On Fri, 30 Sep 2005, Max Bowsher wrote:
> ...
> >> No, clkao's original patch was correct.
> >
> > Yeah, we discussed this on IRC -- I agree.
> >
> >> What's currently committed has the flaw that when opening a non-default
> >> fs-type, repos->fs_type holds a meaningful but incorrect value for a
> >> while,
> >> instead of holding NULL (i.e. unknown) until the real data is known.
> >> I'll revert what was committed, commit clkao's patch instead, and propose
> >> for backport.
> >
> > I can see how you might take that line of thinking, Max. However, I'm
> > missing the part about how the constructor function's creation of
> > svn_repos_t is flawed by setting fs_type to DEFAULT_FS_TYPE. In fact,
> > with
> > that data type constructor function being a part of Subversion's public
> > API,
> > it should return properly initialized data structures, where "properly
> > initialized" means that all default values are already set. It is
> > incohesive and overly coupled to rely on a subsequent function to set the
> > default value. I greatly prefer the r16366 version which you reverted
> > without discussion.
>
> OK, here's my reasoning in more detail:
>
> It is better for a value to be NULL, provoking fast failure if that value is
> unexpectedly read, than to be set to a legal but incorrect value that may
> silently cause inappropriate behaviour.
>
> To give an example - suppose in the future a code path arises where
> create_svn_repos_t() is called on a bdb repository, and the caller neglects
> to set fs_type appropriately. The result would be a svn_repos_t which
> claimed to be fsfs, and would therefore NOT be subjected to the bdb-specific
> locking, whilst actually operating on a bdb repository. This would be
> unsafe, and silent. Far better for the fs_type to be NULL until the correct
> value is known.
>
> Now, you say "with that data type constructor function being a part of
> Subversion's public API", but this is not the case. create_svn_repos_t() is
> a file-local static function, with no visibility outsite the .c file it is
> defined in. Additionally, the only appropriate *default* value is NULL.
>
> I agree that it is unusual for a structure to be initialized in this
> two-stage way, but in this specific instance, it is necessary. The
> allocation and initialization of the path members are common to all uses,
> but each of the three uses of create_svn_repos_t() - create, open, and
> hotcopy - need to initialize the fs_type and format members in different
> ways, in some cases by running code which uses the already-initialized path
> members of the svn_repos_t object.
>
> I have documented this oddity in create_svn_repos_t()'s documentation
> comment, and have verified that none of the 3 callsites fail to complete
> initialization, so I think everything is OK.
Max, thanks for your detailed explanation! I misunderstood your design
goals -- oops!
> Regarding my reversion of the previous version - I did explain in the email
> you replied to why I feel that by doing so I was fixing a totally
> un-bikeshed-y bug. To my mind, it wasn't without discussion, as the
> discussion was one message in length - mine - and I did not forsee any
> conflicting replies.
This is a critical fix for the bindings. Next time, pease revert and
fix in a single atomic commit, so that our bindings are not broken :)
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 30 16:36:04 2005