You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2014/04/29 10:56:47 UTC
Re: svn commit: r1590751 - /subversion/trunk/subversion/svn/svn.c
> Author: philip
> URL: http://svn.apache.org/r1590751
> Log:
> If reading the users config fails, say because $HOME is unreadable,
> provide an empty config rather than a NULL config. This fixes a
> SEGV and allows command line options that override the default
> config to work.
Hi Philip.
I'm wondering why you create the two SVN_CONFIG_CATEGORY_... entries in
the hash? If there's a need for a standard empty config to be created that's more than just an empty hash, we should have a constructor function for it.
The main thing 'svn' does with the config hash is pass it to svn_client_create_context2(), and that already claims to accept NULL. The only other things we do with it are to call svn_hash_gets(), which could be trivially conditionalized, and call svn_cmdline__apply_config_options(). That last call is the only place where we need to actually create a hash. Any reason not to do it there?
What about the other command-line programs, don't they want the same? svnmucc had the same code in it, and svnrdump and svnsync both call svn_config_get_config() without any error handling at all, and presumably all of these should handle these conditions like 'svn' does. svnadmin has a config_dir option but doesn't use it. (We should maybe fix its help to say it's unused, or remove the option, or something.) The other programs don't use a config dir.
I note that you proposed this for back-port to 1.8.
- Julian
>
> * subversion/svn/svn.c
> (sub_main): Provide a fallback empty config.
> Modified: subversion/trunk/subversion/svn/svn.c
> ==============================================================================
> --- subversion/trunk/subversion/svn/svn.c (original)
> +++ subversion/trunk/subversion/svn/svn.c Mon Apr 28 19:11:38 2014
> @@ -2590,9 +2590,15 @@ sub_main(int *exit_code, int argc, const
> if (APR_STATUS_IS_EACCES(err->apr_err)
> || SVN__APR_STATUS_IS_ENOTDIR(err->apr_err))
> {
> + svn_config_t *empty_cfg;
> +
> svn_handle_warning2(stderr, err, "svn: ");
> svn_error_clear(err);
> - cfg_hash = NULL;
> + cfg_hash = apr_hash_make(pool);
> + SVN_ERR(svn_config_create2(&empty_cfg, FALSE, FALSE, pool));
> + svn_hash_sets(cfg_hash, SVN_CONFIG_CATEGORY_CONFIG, empty_cfg);
> + SVN_ERR(svn_config_create2(&empty_cfg, FALSE, FALSE, pool));
> + svn_hash_sets(cfg_hash, SVN_CONFIG_CATEGORY_SERVERS, empty_cfg);
> }
> else
> return err;
>
Re: svn commit: r1590751 - /subversion/trunk/subversion/svn/svn.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
>> I was a bit dubious about changing svn_hash_gets as it is currently a
>> thin wrapper around apr_hash_get.
>
> I didn't mean we should change svn_hash_gets(), I meant to avoid
> calling it if we have no hash.
The reason for creating the empty svn_config_t was avoid having to
insert, and potentially forget, those checks. Perhaps it's easy to
catch the calls in svn.c but it's harder in all the xxx-cmd.c files.
>> I feel a bit uneasy about allowing
>> NULL to be passed everywhere that svn_hash_gets is used. Are there
>> places where we need the hash to exist? I don't know if there are any
>> such places, and if there are then they should already have an explict
>> check.
>>
>>> which could be trivially conditionalized, and call
>>> svn_cmdline__apply_config_options(). That last call is the only place
>>> where we need to actually create a hash. Any reason not to do it
>>> there?
>>
>> svn_cmdline__apply_config_options might be a better place if the
>> svn_hash_gets change is acceptable.
Except it doesn't really work with the current API.
cfg_config = svn_hash_gets(cfg_hash, SVN_CONFIG_CATEGORY_CONFIG);
SVN_ERR(svn_cmdline__apply_config_options(cfg_config, ...));
svn_cmdline__apply_config_options doesn't take in the hash, it takes in
an svn_config_t. If the function were to accept a NULL svn_config_t
that would not be enough for the client as it needs to store the change.
Even if the fuction were modified to return a new svn_config_t when
passed NULL that would not be enough as it doesn't get back into the
hash.
> My main point is that the way this hash (pointer) gets from here to
> the Subversion libraries is through the call to
> svn_client_create_context2(). That is already documented to accept
> NULL. So that seems like the best place to handle any necessary
> default-construction. That also sets the precedent for whether the
> config hash pointers passed around the system generally may or may not
> be null.
The config/context handling is ugly. Pre-1.8 the config was set after
the context was created and those modifications were expected to take
effect. With 1.8 some of the config settings are only ever examined
during the call to svn_client_create_context2(), any subsequent
modification of such settings has no effect. So the svn client attempts
to fully populate the config before creating the context.
Not really sure how best to proceed.
1/ have svn_config_get_config() return the hash of empty svn_config_t
rather than NULL, perhaps with a special error
2/ introduce svn_config_create_config_hash() and call it when
svn_config_get_config() fails
3/ something else?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1590751 - /subversion/trunk/subversion/svn/svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad writes:
>>> URL: http://svn.apache.org/r1590751
>>
>> I'm wondering why you create the two SVN_CONFIG_CATEGORY_... entries
>> in the hash? If there's a need for a standard empty config to be
>> created that's more than just an empty hash, we should have a
>> constructor function for it.
>
> The config API is a bit confusing. At the top level there is no config
> type and practically no API, it's just a hash and apr_hash_get/set. The
> thing that looks like an API, svn_config_t and svn_config_xxx(), is
> really an API to the level below the hash. svn_config_get_config() is a
> bit of an exception.
>
> I suppose we could add a constructor to create "a hash with the right
> content". What should we call such a function? We can't use
> svn_config_create() as that already means something different.
>
> Perhaps it's svn_config_get_config() that should change. Perhaps it
> should map EACCESS and ENOTDIR to some explict config error and not
> return a NULL hash?
>
>> The main thing 'svn' does with the config hash is pass it to
>> svn_client_create_context2(), and that already claims to accept
>> NULL. The only other things we do with it are to call svn_hash_gets(),
>
> I was a bit dubious about changing svn_hash_gets as it is currently a
> thin wrapper around apr_hash_get.
I didn't mean we should change svn_hash_gets(), I meant to avoid calling it if we have no hash.
> I feel a bit uneasy about allowing
> NULL to be passed everywhere that svn_hash_gets is used. Are there
> places where we need the hash to exist? I don't know if there are any
> such places, and if there are then they should already have an explict
> check.
>
>> which could be trivially conditionalized, and call
>> svn_cmdline__apply_config_options(). That last call is the only place
>> where we need to actually create a hash. Any reason not to do it
>> there?
>
> svn_cmdline__apply_config_options might be a better place if the
> svn_hash_gets change is acceptable.
My main point is that the way this hash (pointer) gets from here to the Subversion libraries is through the call to svn_client_create_context2(). That is already documented to accept NULL. So that seems like the best place to handle any necessary default-construction. That also sets the precedent for whether the config hash pointers passed around the system generally may or may not be null.
- Julian
>> What about the other command-line programs, don't they want the same?
>> svnmucc had the same code in it, and svnrdump and svnsync both call
>> svn_config_get_config() without any error handling at all, and
>> presumably all of these should handle these conditions like 'svn'
>> does. svnadmin has a config_dir option but doesn't use it. (We should
>> maybe fix its help to say it's unused, or remove the option, or
>> something.) The other programs don't use a config dir.
>>
>> I note that you proposed this for back-port to 1.8.
>
> I suppose I was being lazy, adding a constructor makes the backport more
> work.
>
> Yes, some other commands are affected. svnadmin is interesting, in
> http://subversion.tigris.org/issues/show_bug.cgi?id=1385 I added a
> comment "svnadmin will error rather than create a missing config" so I
> guess ---config-dir had an effect some time in the past.
>
Re: svn commit: r1590751 - /subversion/trunk/subversion/svn/svn.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
>> Author: philip
>
>> URL: http://svn.apache.org/r1590751
>> Log:
>> If reading the users config fails, say because $HOME is unreadable,
>> provide an empty config rather than a NULL config. This fixes a
>> SEGV and allows command line options that override the default
>> config to work.
>
> Hi Philip.
>
> I'm wondering why you create the two SVN_CONFIG_CATEGORY_... entries
> in the hash? If there's a need for a standard empty config to be
> created that's more than just an empty hash, we should have a
> constructor function for it.
The config API is a bit confusing. At the top level there is no config
type and practically no API, it's just a hash and apr_hash_get/set. The
thing that looks like an API, svn_config_t and svn_config_xxx(), is
really an API to the level below the hash. svn_config_get_config() is a
bit of an exception.
I suppose we could add a constructor to create "a hash with the right
content". What should we call such a function? We can't use
svn_config_create() as that already means something different.
Perhaps it's svn_config_get_config() that should change. Perhaps it
should map EACCESS and ENOTDIR to some explict config error and not
return a NULL hash?
> The main thing 'svn' does with the config hash is pass it to
> svn_client_create_context2(), and that already claims to accept
> NULL. The only other things we do with it are to call svn_hash_gets(),
I was a bit dubious about changing svn_hash_gets as it is currently a
thin wrapper around apr_hash_get. I feel a bit uneasy about allowing
NULL to be passed everywhere that svn_hash_gets is used. Are there
places where we need the hash to exist? I don't know if there are any
such places, and if there are then they should already have an explict
check.
> which could be trivially conditionalized, and call
> svn_cmdline__apply_config_options(). That last call is the only place
> where we need to actually create a hash. Any reason not to do it
> there?
svn_cmdline__apply_config_options might be a better place if the
svn_hash_gets change is acceptable.
> What about the other command-line programs, don't they want the same?
> svnmucc had the same code in it, and svnrdump and svnsync both call
> svn_config_get_config() without any error handling at all, and
> presumably all of these should handle these conditions like 'svn'
> does. svnadmin has a config_dir option but doesn't use it. (We should
> maybe fix its help to say it's unused, or remove the option, or
> something.) The other programs don't use a config dir.
>
> I note that you proposed this for back-port to 1.8.
I suppose I was being lazy, adding a constructor makes the backport more
work.
Yes, some other commands are affected. svnadmin is interesting, in
http://subversion.tigris.org/issues/show_bug.cgi?id=1385 I added a
comment "svnadmin will error rather than create a missing config" so I
guess ---config-dir had an effect some time in the past.