You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2006/07/14 14:54:46 UTC

Re: svn commit: r20648 - in trunk/subversion: include libsvn_subr

dlr@tigris.org wrote:
> Author: dlr
> Date: Thu Jul 13 15:12:08 2006
> New Revision: 20648
> 
> Modified:
>    trunk/subversion/include/svn_config.h
>    trunk/subversion/libsvn_subr/config_file.c
> 
> Log:
> Fix broken API contract and numerous error leaks in
> svn_config_ensure().  The broken API contract resulted an error like
> the following when $HOME isn't accessible:
> 
>   svn: Can't check path '/home/thiru/.subversion': Permission denied

How is that a broken API contract?  The contact mentions exactly two cases
in which errors should *not* be returned:

  - something exists but is the wrong kind
  - you fail while trying to create something

The "permission denied" case you site fits neither of those.  So, where's
the problem?  (I mean, besides those error leaks, which really *are* no-nos.)

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: svn commit: r20648 - in trunk/subversion: include libsvn_subr

Posted by "C. Michael Pilato" <cm...@collab.net>.
C. Michael Pilato wrote:
> Daniel Rall wrote:
> 
>>This violates the spirit of this API -- we should not return an error
>>due to insufficient permissions when we can bow out gracefully.  Can
>>you suggest a better phrasing for the doc string which would make this
>>more explicit?

[cmpilato's blather omitted in a weak attempt to salvage his pride]

Dude.  Ignore all that.  I mistakenly thought that svn_config_ensure was
about both writing *and* reading the configuration directory.  Carry on.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: svn commit: r20648 - in trunk/subversion: include libsvn_subr

Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Rall wrote:
> On Fri, 14 Jul 2006, C. Michael Pilato wrote:
> ...
> 
>>>   trunk/subversion/include/svn_config.h
>>>   trunk/subversion/libsvn_subr/config_file.c
>>>
>>>Log:
>>>Fix broken API contract and numerous error leaks in
>>>svn_config_ensure().  The broken API contract resulted an error like
>>>the following when $HOME isn't accessible:
>>>
>>>  svn: Can't check path '/home/thiru/.subversion': Permission denied
>>
>>How is that a broken API contract?  The contact mentions exactly two cases
>>in which errors should *not* be returned:
>>
>>  - something exists but is the wrong kind
>>  - you fail while trying to create something
>>
>>The "permission denied" case you site fits neither of those.  So, where's
>>the problem?  (I mean, besides those error leaks, which really *are* no-nos.)
> 
> 
> Mike, thanks for the review.  The API states:
> 
>  "Also don't error if trying to create something and failing -- it's
>  okay for the config area or its contents not to be created."
> 
> While we're not performing a specific write operation where this error
> occurs (svn_io_check_path() on CONFIG_DIR), we are returning an error
> due to insufficient permissions to the file system where we plan to
> subsequently create our config area and templates.
> 
> This violates the spirit of this API -- we should not return an error
> due to insufficient permissions when we can bow out gracefully.  Can
> you suggest a better phrasing for the doc string which would make this
> more explicit?

I see as different "try to create something new but can't" and "try to read
something existing but can't".  If Subversion can't create from scratch a
new configuration area, it's okay to silently move along, because its
behavior will fallback to the exact same values that would have been written
to that new configuration area.  However, if Subversion can't read from an
existing configuration area, it's *not* okay to silently move along, because
the default values it chooses could differ from the possibly hand-tweaked
configuration in the area it tried to read but couldn't.  I think users will
want to know about such a situation, so they can either fix the permissions
on the configuration directory, or use --config-dir to tell the Subversion
binaries to use a different one.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: svn commit: r20648 - in trunk/subversion: include libsvn_subr

Posted by Daniel Rall <dl...@collab.net>.
On Fri, 14 Jul 2006, C. Michael Pilato wrote:
...
> >    trunk/subversion/include/svn_config.h
> >    trunk/subversion/libsvn_subr/config_file.c
> > 
> > Log:
> > Fix broken API contract and numerous error leaks in
> > svn_config_ensure().  The broken API contract resulted an error like
> > the following when $HOME isn't accessible:
> > 
> >   svn: Can't check path '/home/thiru/.subversion': Permission denied
> 
> How is that a broken API contract?  The contact mentions exactly two cases
> in which errors should *not* be returned:
> 
>   - something exists but is the wrong kind
>   - you fail while trying to create something
> 
> The "permission denied" case you site fits neither of those.  So, where's
> the problem?  (I mean, besides those error leaks, which really *are* no-nos.)

Mike, thanks for the review.  The API states:

 "Also don't error if trying to create something and failing -- it's
 okay for the config area or its contents not to be created."

While we're not performing a specific write operation where this error
occurs (svn_io_check_path() on CONFIG_DIR), we are returning an error
due to insufficient permissions to the file system where we plan to
subsequently create our config area and templates.

This violates the spirit of this API -- we should not return an error
due to insufficient permissions when we can bow out gracefully.  Can
you suggest a better phrasing for the doc string which would make this
more explicit?

- Dan