[PATCH] Replace svn_io_set_file_read_write_carefully

["D.J. Heap" <dj...@gmail.com>]

Any objections to the attached patch?  Its a step toward supporting
Apache 2.2/APR 1.x on Win32.

Log:
Replace svn_io_set_file_read_write_carefully with
svn_io_set_file_read_write_perms which has been updated to work with
APR 1.x behavior on Win32.

It's behavior on unix style OS' is unchanged, but on Win32 it now just
forwards the calls to the appropriate svn_io_set_file_read_* functions.

Setting permissions on Win32 is unimplemented in APR, and Win32's
inherited ACL's do the right thing anyway on filesystems that support
permissions.

See this email thread for previous discussion:
http://svn.haxx.se/dev/archive-2006-02/0747.shtml

* subversion/include/svn_io.h
  Deprecate svn_io_set_file_read_write_carefully and declare
  new svn_io_set_file_read_write_perms function.

* subversion/libsvn_wc/props.c
  (svn_wc_prop_set2): Use new function.

* subversion/libsvn_wc/log.c
  (install_committed_file): Ditto.

* subversion/libsvn_wc/adm_ops.c
  (svn_wc_add_lock, svn_wc_remove_lock): Ditto.

* subversion/libsvn_wc/translate.c
  (svn_wc__maybe_set_read_only): Ditto.

* subversion/libsvn_subr/io.c
  (svn_io_set_file_read_write_perms): Implement new function.
  (svn_io_set_file_read_write_carefully): Forward call to new function.


DJ

Re: [PATCH] Replace svn_io_set_file_read_write_carefully

["D.J. Heap" <dj...@gmail.com>]

On 6/4/06, Branko Čibej <br...@xbc.nu> wrote:
> D.J. Heap wrote:
> > Something like this?  It passes ra_local tests on Win32 and FC5.
> Yes! Thank you.
>


Commited in r19924 and nominated on the 1.4.x branch.

Thanks for the suggestions and review!

DJ

Re: [PATCH] Replace svn_io_set_file_read_write_carefully

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> Something like this?  It passes ra_local tests on Win32 and FC5.
Yes! Thank you.

(The next step would be to fix apr_file_attrs_set to do something
similar; _that_ implementation is my fault, too :( But this should work
correctly.)

[...]

> @@ -321,8 +322,10 @@
>   *
>   * @a path is the utf8-encoded path to the file.  If @a enable_write
>   * is @c TRUE, then make the file read-write.  If @c FALSE, make it
> - * write-only.  If @a ignore_enoent is @c TRUE, don't fail if the target
> + * read-only.  If @a ignore_enoent is @c TRUE, don't fail if the target
>   * file doesn't exist.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
>   
"for backward compatibility with the 1.4 API" (or 1.3, if this gets
back-ported to the 1.4 branch before the release).
[...]

> +/* This is a helper function for the svn_io_set_file_read* functions
> +   that attempts to honor the users umask when dealing with
> +   permission changes. */
> +static svn_error_t *
> +io_set_file_perms(const char *path,
> +                  svn_boolean_t change_readwrite,
> +                  svn_boolean_t enable_write,
> +                  svn_boolean_t change_executable,
> +                  svn_boolean_t executable,
> +                  svn_boolean_t ignore_enoent,
> +                  apr_pool_t *pool)
>   
You could wrap this whole function into #ifndef WIN32, since it's not
going to be used on that platform.
[...]

-- Brane

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Replace svn_io_set_file_read_write_carefully

["D.J. Heap" <dj...@gmail.com>]

Something like this?  It passes ra_local tests on Win32 and FC5.

Log:
Deprecate svn_io_set_file_read_write_carefully and update
the svn_io_set_file_read* functions to handle permissions on
unix or just set the appropriate attributes on Windows.

See these email threads for previous discussion:
http://svn.haxx.se/dev/archive-2006-02/0747.shtml
http://svn.haxx.se/dev/archive-2006-06/0093.shtml

* subversion/include/svn_io.h
 Deprecate svn_io_set_file_read_write_carefully.

* subversion/libsvn_wc/props.c
 (svn_wc_prop_set2): Use appropriate svn_io_set_file_read_* function
  rather than svn_io_set_file_read_write_carefully..

* subversion/libsvn_wc/log.c
 (install_committed_file): Ditto.

* subversion/libsvn_wc/adm_ops.c
 (svn_wc_add_lock, svn_wc_remove_lock): Ditto.

* subversion/libsvn_wc/translate.c
 (svn_wc__maybe_set_read_only): Ditto.

* subversion/libsvn_subr/io.c
  (io_set_file_perms): New function to help deal with file permissions
  on unix.
  (svn_io_set_file_read_write_carefully): Just forward call to appropriate
  svn_io_set_file_read* functions.
  (svn_io_set_file_read_only): Call io_set_file_perms helper on unix but
  just set the appropriate attributes on Windows.
  (svn_io_set_file_read_write): Ditto.
  (svn_io_set_file_executable): Call io_set_file_perms helper on unix but
  just return on Windows.

DJ

Re: [PATCH] Replace svn_io_set_file_read_write_carefully

["D.J. Heap" <dj...@gmail.com>]

On 6/3/06, Branko Čibej <br...@xbc.nu> wrote:
> D.J. Heap wrote:
> > Any objections to the attached patch?  Its a step toward supporting
> > Apache 2.2/APR 1.x on Win32.
> [First of all, I must apologise to you for dropping the ball on solving
> this problem. What follows is not intended as a criticism of your patch,
> but a discussion of the perms-modifying code in general.]


No problem.


[snip]
>
> Now here's my proposal for remedying this situation:
>
> 1. Create a *private*, Unix-specific function in libsvn_subr that does
> the jumping-through-hoops that svn_io_set_file_executable and
> svn_io_set_file_read_write_carefully do now.
>
> 2. Reimplement svn_io_set_file_executable, svn_io_set_file_read_write
> and svn_io_set_file_read_only so that, on Unix, they call this new
> hoop-jumping function. On Windows, they should either do nothing (as
> ...set_file_executable does now) or call apr_file_attrs_set.
>
> 3. Deprecate svn_io_set_file_read_write_carefully and reimplement it as
> a wrapper for svn_io_set_file_read_(write|only).
>

Ok, I'll try to rework it to that end.

Thanks!

DJ

Re: [PATCH] Replace svn_io_set_file_read_write_carefully

[Branko Čibej <br...@xbc.nu>]

D.J. Heap wrote:
> Any objections to the attached patch?  Its a step toward supporting
> Apache 2.2/APR 1.x on Win32.
[First of all, I must apologise to you for dropping the ball on solving
this problem. What follows is not intended as a criticism of your patch,
but a discussion of the perms-modifying code in general.]


IMNSHO, there is not a single logical reason to keep this function in
the code, whether we call it blah_blah_carefully or blah_blah_perms.

Originally, we had svn_io_set_file_read_write and
svn_io_set_file_read_only, which called apr_file_attrs_set, and were
used only on the files within the .svn/ directory tree. Because the
umask(2) semantics are slightly broken (i.e., there's no way to figure
out the current process' umask without changing it, and it _is_ a
process-global parameter), apr_file_attrs_set doesn't take the current
umask into account. As a result, svn_io_set_file_read_write would make a
file globally writable on Unix-like systems. This was deemed acceptable
for working-copy-metadata files.

At about the same time (historically speaking :) we decided to start
fiddling with the executable bit. Obviously, what apr_file_attrs_set
does was no longer acceptable, so svn_io_set_file_executable jumps
through hoops in order to figure out what the umask might be relatively
safely. Those hoops don't exist on Windows, but that didn't matter,
because we ignore svn:executable there anyway.

Then along came locking support and the need to manipulate the write
protection bits on "real" working-copy files. The current semantics of
svn_io_set_file_read_(write|only) were no longer sufficient, so along
came svn_io_set_file_read_write_carefully, which is a copy-paste clone
of svn_io_set_file_executable, but tweaked to handle the write perms
instead of execute perms. (I won't go into the copy-paset part here,
grrr, since it's as much my fault as anyone else's for not noticing that
during commit review.)

The real trouble is that this new function had the exact semantics that
svn_io_set_file_read_(write|only) should have had from the beginning.
But, instead of it being a private implementation of those semantics for
Unix only, it's a public API that's only used in the locking-support
code, while other parts of the WC-management code still uses the older
two functions. The fact that it breaks on Windows with newer versions of
APR is only a minor side-effect at this time.


Now here's my proposal for remedying this situation:

1. Create a *private*, Unix-specific function in libsvn_subr that does
the jumping-through-hoops that svn_io_set_file_executable and
svn_io_set_file_read_write_carefully do now.

2. Reimplement svn_io_set_file_executable, svn_io_set_file_read_write
and svn_io_set_file_read_only so that, on Unix, they call this new
hoop-jumping function. On Windows, they should either do nothing (as
...set_file_executable does now) or call apr_file_attrs_set.

3. Deprecate svn_io_set_file_read_write_carefully and reimplement it as
a wrapper for svn_io_set_file_read_(write|only).


Yes, this would cause a somewhat larger overhead in WC metadata
management on Unix. On the other hand, we'd really only have to jump
through hoops to figure out the current umask once per WC, and could
cache the result. That's a low-priority optimisation though, cleaning up
the API (and certainly supporting httpd-2.2 on Windows) is a higher
priority.


-- Brane

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org