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.
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.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 30 12:17:12 2005