On 13.04.2011 03:14, C. Michael Pilato wrote:
On 04/12/2011 05:11 PM, arfre...@apache.org wrote:
Author: arfrever
Date: Tue Apr 12 21:11:46 2011
New Revision: 1091573
URL: http://svn.apache.org/viewvc?rev=1091573&view=rev
Log:
Followup to r1090784:
* build.conf
(libsvn_ra.libs): Add libsvn_fs_util.
Whoa. This doesn't feel right to me. Well, let me clarify, as I can see
why r1090784 makes this necessary.
It's r1090784 which looks severely problematic to me. The RA loader module
is not permitted to access svn_fs_ functions directly! It may only call the
vtable functions provided by the individual RA plugins. Of those plugins,
ra_local is the only one permitted to call svn_fs_ functions.
And now that I'm looking at the svn_fs_get_cache_config(), I'm feeling wrong
about that, too. If this cache stuff is an FSFS-only thing, then it needs
to live inside libsvn_fs_fs. libsvn_fs_util exists *only* for use by FS
implementations (libsvn_fs_fs and libsvn_fs_base, today) which need to share
utility functions.
Sorry I didn't catch this stuff earlier (yes, guilty of "well, *someone*
must be reviewing this code" syndrome), but I'm seeing what appears to be a
series of screaming layering violations here.
The code in question has evolved over many months so it is
very possible that the name of svn_fs_get_cache_config()
and friends is no longer appropriate. The same goes for the
place that this has been implemented.
From a design perspective, I think it is perfectly to o.k. to
expose resource limits on the server UI level. The fact that FSFS
is currently the only part of the server that uses these settings
does not change, IMO, the fact that they are part of the UI.
Here is where a design issue with our RA layer concept comes
into play: "server" as in "repository content access provider"
can be almost any svn executable: apache/mod_dav_svn,
svnserve, svn, svnadmin, ...
Maybe we should simply move the function in question to libsvn_subr
and rename them properly.
Comments?
-- Stefan^2.
