Philip Martin <philip@codematters.co.uk> writes:
> Subversion defines apr_copy_file and apr_transfer_file_contents in the
> file subversion/libsvn_subr/io.c. These functions copy both the file
> contents and the file permissions.
Actually, it's worse than that. I think apr_transfer_file_contents()
has a bug. Since apr_copy_file() is just a wrapper around it, that
means they both have the bug. Here's the relevant code excerpt from
apr_transfer_file_contents():
[...]
/* Get its size. */
apr_err = apr_file_info_get (&finfo, APR_FINFO_MIN, s);
if (apr_err)
{
apr_file_close (s); /* toss any error */
return apr_err;
}
else
perms = finfo.protection;
/* Open dest file. */
apr_err = apr_file_open (&d, dst, flags, perms, pool);
[...]
The first comment is misleading. It claims we're interested in the
size, but all we ever do with `finfo' is extract the perms from it and
use them to open the dst file. We never use `perms' or `finfo' again.
Therefore, not only is the comment wrong, but the APR_FINFO_MIN flag
is wrong. Don't we want APR_FINFO_PROT instead? Here are their
definitions in apr_file_info.h:
#define APR_FINFO_MIN 0x00008170 /**< type, mtime, ctime, atime, size */
#define APR_FINFO_PROT 0x00700000 /**< all protections */
Am I missing something here?
> Is copying the file permissions a good idea? When a file is created
> it can be created with permissions APR_OS_DEFAULT. apr_copy_file and
> apr_transfer_file_contents will create a file if necessary but always
> set the permissions to match those of the source file. There is no
> easy way to restore the default permissions on the destination file.
>
> I can see that at times it might be necessary to atomically copy and
> set permissions, so simply removing the permission copying is not a
> good idea. I am not sure what the interface should be: perhaps two
> functions apr_copy_file and apr_copy_file_contents, or perhaps one
> function with a flag.
Maybe apr_copy_file() and its helper should take a flag that indicates
either the perms to use for the new file, or to use the same perms as
the old file (we could add a new flag value for that if necessary).
How does that sound? Any thoughts from APR people?
> What is the status of these "apr in subversion" functions?
I think these functions were meant to go into APR eventually, I'm not
sure why they haven't yet. If there's currently a bug, obviously that
would be one reason not to put them in APR yet. :-)
I think apr_transfer_file_contents is intended to be a non-public
helper, but not sure about that.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:00 2006