[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: apr_copy_file and apr_transfer_file_contents

From: Karl Fogel <kfogel_at_newton.ch.collab.net>
Date: 2002-01-28 19:24:12 CET

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

This is an archived mail posted to the Subversion Dev mailing list.