Re: [PATCH] Win32 File Permission Fixes

Comments below.

Jay Freeman (saurik) wrote:

>OK, here is the end result of the various Win32 permission fixes (and
>one NULL initialization problem):
>
>
>* ./subversion/include/svn_io.h (svn_io_set_file_read_write): Added
>prototype for a new svn_io_set_file_read_write() function that removes
>the read-only flag from the specified file.
>
>* ./subversion/libsvn_wc/adm_files.c (close_adm_file): When a file is
>overwritten by another file on Win32, it is important that the file not
>be read-only. Before performing the rename, close_adm_file() now
>removes the read-only flag on the destination.
>
>(svn_wc__remove_adm_file): On Win32, files that are marked read-only
>cannot be deleted. Before performing the delete,
>svn_wc__remove_adm_file now removes the read-only flag on the
>destination.
>
>* ./subversion/libsvn_wc/get_editor.c (svn_wc_install_file): There was a
>small bug where the fresh_keywords_value pointer was failed to be
>initialized to NULL.
>
>* ./subversion/libsvn_subr/io.c (svn_io_set_file_read_write):
>Implemented a new function that removes the read-only flag from the
>specified file using apr_file_attrs_set().
>
>
>
>Index: .\subversion\include\svn_io.h
>===================================================================
>--- .\subversion\include\svn_io.h
>+++ .\subversion\include\svn_io.h Sun Feb 10 17:54:56 2002
>@@ -144,6 +144,11 @@
> */
> svn_error_t *svn_io_set_file_read_only (const char *path, apr_pool_t
>*pool);
>
>+/* Make a file as writable as the operating system allows.
>+ * PATH is the path to the file.
>+ */
>+svn_error_t *svn_io_set_file_read_write (const char *path, apr_pool_t
>*pool);
>+
>
> /* Read a line from FILE into BUF, but not exceeding *LIMIT bytes.
> * Does not include newline, instead '\0' is put there.
>Index: .\subversion\libsvn_wc\adm_files.c
>===================================================================
>--- .\subversion\libsvn_wc\adm_files.c
>+++ .\subversion\libsvn_wc\adm_files.c Mon Feb 11 03:36:56 2002
>@@ -645,6 +645,7 @@
> ...)
> {
> apr_status_t apr_err = 0;
>+ svn_error_t *err = NULL;
> int components_added;
> va_list ap;
>
>@@ -681,6 +682,11 @@
> v_extend_with_adm_name (tmp_path, extension, 1, pool, ap);
> va_end (ap);
>
>+ /* Remove read-only flag on destination. */
>+ err = svn_io_set_file_read_write (path->data, pool);
>+ if (err && !APR_STATUS_IS_ENOENT(err->apr_err))
>+ return err;
>+
>
I don't like looking through the svn_error_t. Might it be better to have:

svn_error_t *svn_io_set_file_read_write (const char *path,
                                         svn_boolean_t ignore_enoent,
                                         apr_pool_t *pool)

and similarily for svn_io_set_file_read_only? Then you could use just
SVN_ERR here.

>
> /* Rename. */
> apr_err = apr_file_rename (tmp_path->data, path->data, pool);
> if (APR_STATUS_IS_SUCCESS(apr_err))
>@@ -998,6 +1004,9 @@
> va_start (ap, pool);
> components_added = v_extend_with_adm_name (path, NULL, 0, pool, ap);
> va_end (ap);
>+
>+ /* Remove read-only flag on path. */
>+ SVN_ERR(svn_io_set_file_read_write (path->data, pool));
>
> apr_err = apr_file_remove (path->data, pool);
> if (apr_err)
>Index: .\subversion\libsvn_wc\get_editor.c
>===================================================================
>--- .\subversion\libsvn_wc\get_editor.c
>+++ .\subversion\libsvn_wc\get_editor.c Mon Feb 11 03:34:56 2002
>@@ -1524,7 +1524,7 @@
> {
> /* Did we get a new keywords value passed into this routine? */
> int i;
>- svn_stringbuf_t *fresh_keywords_value;
>+ svn_stringbuf_t *fresh_keywords_value = NULL;
>
> /* Rats, here's one case where it would be *nice* to have a
> hash instead of an array. */
>Index: .\subversion\libsvn_subr\io.c
>===================================================================
>--- .\subversion\libsvn_subr\io.c
>+++ .\subversion\libsvn_subr\io.c Mon Feb 11 03:29:12 2002
>@@ -378,6 +378,34 @@
> return SVN_NO_ERROR;
> }
>
>+svn_error_t *
>+svn_io_set_file_read_write (const char *path, apr_pool_t *pool)
>+{
>+ apr_status_t status;
>+ apr_fileattrs_t attributes;
>+
>+#ifdef SVN__APR_FILE_ATTRS_GET
>+ /* This is what he would do if the get function was available in APR.
>If
>+ the attribute removing stuff gets in to APR and we don't do this,
>then
>+ removing read-only will remove executable. Since we don't ever set
>+ executable at present that is not a big problem */
>+ status = apr_file_attrs_get (path, &attributes, pool);
>+ if (!APR_STATUS_IS_SUCCESS(status))
>+ return svn_error_createf (status, 0, NULL, pool,
>+ "failed to get attributes for file '%s'",
>path);
>+#else
>+ attributes = 0;
>+#endif
>
Blech. I think the signature for apr_file_attrs_set should change
instead (we can do that, since presumably nobody else is using that
function yet):

apr_status_t apr_file_attrs_set (const char *path,
                                 apr_uint32_t attribures,
                                 apr_uint32_t attr_mask,
                                 apr_pool_t *pool);

Here, ATTR_MASK tells which bits in ATTRIBUTES are valid.

(Of course, apr_file_attrs_get should be added for symmetry anyway).

>
>+
>+ attributes &= !APR_FILE_ATTR_READONLY;
>
Nope: you want "attributes &= ~APTR_FILE_ATTR_READONLY". Well, actually
you want the changed signature, but failing that, 1's complement is
surely better than logical negation. :-)

>
>+ status = apr_file_attrs_set (path, attributes, pool);
>+ if (!APR_STATUS_IS_SUCCESS(status))
>+ return svn_error_createf (status, 0, NULL, pool,
>+ "failed to set file '%s' read-write",
>path);
>+
>+ return SVN_NO_ERROR;
>+}
>+
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>For additional commands, e-mail: dev-help@subversion.tigris.org
>

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org