> -----Original Message-----
> From: Philip Martin [mailto:philip_at_codematters.co.uk]
> Sent: vrijdag 19 september 2008 14:53
> To: dev_at_subversion.tigris.org
> Cc: rhuijben_at_tigris.org
> Subject: Re: svn commit: r33178 - in trunk/subversion: libsvn_client
> libsvn_subr
>
> rhuijben_at_tigris.org writes:
>
> > Author: rhuijben
> > Date: Fri Sep 19 01:41:23 2008
> > New Revision: 33178
> >
> > Log:
> > Avoid calling apr_status with APR_FINFO_PROT | APR_FINFO_OWNER on
> Windows, as
> > this call is 'incredibly expensive, but accurate' and the result is
> ignored
> > anyway.
> >
> > * subversion/libsvn_subr/io.c
> > (dir_make): Skip trying to set the unavailable sgid flag for WIN32.
> > This avoids an 'incredibly expensive' ACL retrieval in apr.
> > * subversion/libsvn_client/export.c
> > (copy_versioned_files): Call svn_io_dir_make with APR_OS_DEFAULT
> permissions
> > to avoid an 'incredibly expensive' ACL retrieval in apr.
>
> Please put comments describing the code in the code and not in the log
> message. The log message should describe the change:
>
> "Avoid unnecessary frobnication on Windows"
>
> the comments in the code should describe the code:
>
> /* No need to frobnicate on Windows as it happens automatically
> when the sun goes down. */
>
> >
> > Modified:
> > trunk/subversion/libsvn_client/export.c
> > trunk/subversion/libsvn_subr/io.c
> >
> > Modified: trunk/subversion/libsvn_client/export.c
> > URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/export.
> c?pathrev=33178&r1=33177&r2=33178
> >
> =======================================================================
> =======
> > --- trunk/subversion/libsvn_client/export.c Thu Sep 18 23:00:28 2008
> (r33177)
> > +++ trunk/subversion/libsvn_client/export.c Fri Sep 19 01:41:23 2008
> (r33178)
> > @@ -249,8 +249,12 @@ copy_versioned_files(const char *from,
> > /* Try to make the new directory. If this fails because the
> > directory already exists, check our FORCE flag to see if we
> > care. */
> > +#ifndef WIN32
> > SVN_ERR(svn_io_stat(&finfo, from, APR_FINFO_PROT, pool));
> > err = svn_io_dir_make(to, finfo.protection, pool);
> > +#else
> > + err = svn_io_dir_make(to, APR_OS_DEFAULT, pool);
> > +#endif
>
> How is a non-Windows programmer supposed to understand why the Windows
> code is different? Please add a comment.
>
> > if (err)
> > {
> > if (! APR_STATUS_IS_EEXIST(err->apr_err))
> >
> > Modified: trunk/subversion/libsvn_subr/io.c
> > URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/io.c?path
> rev=33178&r1=33177&r2=33178
> >
> =======================================================================
> =======
> > --- trunk/subversion/libsvn_subr/io.c Thu Sep 18 23:00:28 2008
> (r33177)
> > +++ trunk/subversion/libsvn_subr/io.c Fri Sep 19 01:41:23 2008
> (r33178)
> > @@ -2800,6 +2800,7 @@ dir_make(const char *path, apr_fileperms
> > }
> > #endif
> >
> > +#ifndef WIN32
> > if (sgid)
> > {
> > apr_finfo_t finfo;
> > @@ -2811,6 +2812,7 @@ dir_make(const char *path, apr_fileperms
> > if (!status)
> > apr_file_perms_set(path_apr, finfo.protection | APR_GSETID);
> > }
> > +#endif
>
> Same question as above.
Hmm... perhaps this should tell you that the code is not supported on
windows? There is no sgid.
Windows implements inheritable access control lists (ACL) on all disk nodes.
> It seems odd that it is expensive to get permissions if the returned
> values are useless. Is this an apr bug? Could we change apr so that
> the conditional code in Subversion is unnecessary?
APR accesses the security management api to retrieve all ACL information and
then creates a unix like mask for you. This allows you to check whether the
user can read, write or execute like you would do on a unix operating system
that only has an uid, gid and mask.
But for this it requires access to the domain controller if your windows pc
is in a domain.
I don't think this is fixable in apr without completely redesigning the file
security api. You can represent an umask as a non inheritable ACL, but not
the other way around.
apr_file_perms_set and the permission argument on svn_io_dir_make are
implemented as no-op on windows. So retrieving this mask is not very useful.
Adding extra wrappers in apache wouldn't be very useful as apache did
already wrap this.
But see file_io/win32/filestat.c around line 230 for more information:
====================
if (wanted & (APR_FINFO_PROT | APR_FINFO_OWNER))
{
/* On NT this request is incredibly expensive, but accurate.
* Since the WinNT-only functions below are protected by the
* (apr_os_level < APR_WIN_NT) case above, we need no extra
* tests, but remember GetNamedSecurityInfo & GetSecurityInfo
* are not supported on 9x.
*/
=====================
Bert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-19 15:39:57 CEST