You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2008/08/08 11:28:01 UTC
Re: svn commit: r683626 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/repos.c
On Thu, Aug 07, 2008 at 03:12:00PM -0000, Jeff Trawick wrote:
> --- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
> +++ httpd/httpd/trunk/modules/dav/fs/repos.c Thu Aug 7 08:12:00 2008
> @@ -1475,10 +1475,8 @@
> /* append this file onto the path buffer (copy null term) */
> dav_buffer_place_mem(pool, &fsctx->path1, dirent.name, len + 1, 0);
>
> -
> - /* ### Optimize me, dirent can give us what we need! */
> status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
> - APR_FINFO_NORM | APR_FINFO_LINK, pool);
> + APR_FINFO_TYPE | APR_FINFO_LINK, pool);
> if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
> /* woah! where'd it go? */
> /* ### should have a better error here */
>
Nit pick: on Unix APR_FINFO_PROT is needed too, to support the
executable property... but it gets picked up by stat anyway, and Win32
doesn't expose that property.
joe
Re: svn commit: r683626 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/repos.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Aug 11, 2008 at 3:20 PM, Joe Orton <jo...@redhat.com> wrote:
> On Fri, Aug 08, 2008 at 09:42:01AM -0400, Jeff Trawick wrote:
> > On Fri, Aug 8, 2008 at 5:28 AM, Joe Orton <jo...@redhat.com> wrote:
> > > On Thu, Aug 07, 2008 at 03:12:00PM -0000, Jeff Trawick wrote:
> > > > --- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
> > > > +++ httpd/httpd/trunk/modules/dav/fs/repos.c Thu Aug 7 08:12:00 2008
> > > > @@ -1475,10 +1475,8 @@
> > > > /* append this file onto the path buffer (copy null term) */
> > > > dav_buffer_place_mem(pool, &fsctx->path1, dirent.name, len
> + 1,
> > > 0);
> > > >
> > > > -
> > > > - /* ### Optimize me, dirent can give us what we need! */
> > > > status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
> > > > - APR_FINFO_NORM | APR_FINFO_LINK, pool);
> > > > + APR_FINFO_TYPE | APR_FINFO_LINK, pool);
> > > > if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
> > > > /* woah! where'd it go? */
> > > > /* ### should have a better error here */
> > > >
> > >
> > > Nit pick: on Unix APR_FINFO_PROT is needed too, to support the
> > > executable property....
> >
> > plz show me where; I'm blind; thanks!
>
> If I'm understanding things correctly, this stat() call fetches file
> attributes which will be used when returning property information via
> dav_fs_insert_prop.
>
It took me a while, but I found out how ;)
dav_fs_internal_walk() does
fsctx.res1.info = &fsctx.info1;
and much later
/* point the callback's resource at our structure */
fsctx.wres.resource = &fsctx.res1;
so this call to a callback (dav_propfind_walker) makes available the info
gathered by this apr_stat():
if (fsctx->info1.finfo.filetype == APR_REG) {
/* call the function for the specified dir + file */
if ((err = (*params->func)(&fsctx->wres,
wres has addressability to new stat <<<<
DAV_CALLTYPE_MEMBER)) != NULL) {
/* ### maybe add a higher-level description? */
break;
}
}
(Ouch!) This was complicated enough that my naive attempts to see who cared
about info1.finfo were useless.
Thanks for noticing/fixing!
Re: svn commit: r683626 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/repos.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Aug 11, 2008 at 08:20:40PM +0100, Joe Orton wrote:
> I think that something like this is the way to go: (against 2.2.x since
> my trunk install is currently refusing to do anything DAVy)
I committed a version of that with the logic, um, improved, as r685112,
and am +1 for backport of r683626 + r685112 to 2.2.x.
joe
Re: svn commit: r683626 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/repos.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Aug 08, 2008 at 09:42:01AM -0400, Jeff Trawick wrote:
> On Fri, Aug 8, 2008 at 5:28 AM, Joe Orton <jo...@redhat.com> wrote:
> > On Thu, Aug 07, 2008 at 03:12:00PM -0000, Jeff Trawick wrote:
> > > --- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
> > > +++ httpd/httpd/trunk/modules/dav/fs/repos.c Thu Aug 7 08:12:00 2008
> > > @@ -1475,10 +1475,8 @@
> > > /* append this file onto the path buffer (copy null term) */
> > > dav_buffer_place_mem(pool, &fsctx->path1, dirent.name, len + 1,
> > 0);
> > >
> > > -
> > > - /* ### Optimize me, dirent can give us what we need! */
> > > status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
> > > - APR_FINFO_NORM | APR_FINFO_LINK, pool);
> > > + APR_FINFO_TYPE | APR_FINFO_LINK, pool);
> > > if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
> > > /* woah! where'd it go? */
> > > /* ### should have a better error here */
> > >
> >
> > Nit pick: on Unix APR_FINFO_PROT is needed too, to support the
> > executable property....
>
> plz show me where; I'm blind; thanks!
If I'm understanding things correctly, this stat() call fetches file
attributes which will be used when returning property information via
dav_fs_insert_prop.
So I think that the stat() call should mask in all the attribute types
which are used in dav_fs_insert_prop: ctime, mtime, size, and prot iff
!WIN32.
Here is a good demo - change APR file_io/unix/filestat.c as below:
static void fill_out_finfo(apr_finfo_t *finfo, struct_stat *info,
apr_int32_t wanted)
{
#if 0
finfo->valid = APR_FINFO_MIN | APR_FINFO_IDENT | APR_FINFO_NLINK
| APR_FINFO_OWNER | APR_FINFO_PROT;
#else
finfo->valid = wanted;
#endif
i.e. lose all the file attributes which come for free with stat() but
weren't explicitly requested; you'll see that mod_dav no longer returns
the executable property.
(with cadaver, for an existing foo.txt on the server, "chexec + foo.txt"
then see whether the "*" shows up next to foo.txt in a subsequent "ls")
I think that something like this is the way to go: (against 2.2.x since
my trunk install is currently refusing to do anything DAVy)
--- repos.c (revision 683918)
+++ repos.c (working copy)
@@ -119,9 +119,19 @@
** Does this platform support an executable flag?
**
** ### need a way to portably abstract this query
+**
+** DAV_FINFO_MASK gives the appropriate mask to use for the stat call
+** used to get file attributes.
*/
#ifndef WIN32
#define DAV_FS_HAS_EXECUTABLE
+#define DAV_FINFO_MASK (APR_FINFO_LINK | APR_FINFO_TYPE | APR_FINFO_INODE | \
+ APR_FINFO_SIZE | APR_FINFO_CTIME | APR_FINFO_MTIME)
+#else
+/* as above plus APR_FINFO_PROT */
+#define DAV_FINFO_MASK (APR_FINFO_LINK | APR_FINFO_TYPE | APR_FINFO_INODE | \
+ APR_FINFO_SIZE | APR_FINFO_CTIME | APR_FINFO_MTIME \
+ APR_FINFO_PROT)
#endif
/*
@@ -1482,7 +1492,7 @@
/* ### Optimize me, dirent can give us what we need! */
status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
- APR_FINFO_NORM | APR_FINFO_LINK, pool);
+ DAV_FINFO_MASK, pool);
if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
/* woah! where'd it go? */
/* ### should have a better error here */
Re: svn commit: r683626 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/repos.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Aug 8, 2008 at 5:28 AM, Joe Orton <jo...@redhat.com> wrote:
> On Thu, Aug 07, 2008 at 03:12:00PM -0000, Jeff Trawick wrote:
> > --- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
> > +++ httpd/httpd/trunk/modules/dav/fs/repos.c Thu Aug 7 08:12:00 2008
> > @@ -1475,10 +1475,8 @@
> > /* append this file onto the path buffer (copy null term) */
> > dav_buffer_place_mem(pool, &fsctx->path1, dirent.name, len + 1,
> 0);
> >
> > -
> > - /* ### Optimize me, dirent can give us what we need! */
> > status = apr_stat(&fsctx->info1.finfo, fsctx->path1.buf,
> > - APR_FINFO_NORM | APR_FINFO_LINK, pool);
> > + APR_FINFO_TYPE | APR_FINFO_LINK, pool);
> > if (status != APR_SUCCESS && status != APR_INCOMPLETE) {
> > /* woah! where'd it go? */
> > /* ### should have a better error here */
> >
>
> Nit pick: on Unix APR_FINFO_PROT is needed too, to support the
> executable property...
plz show me where; I'm blind; thanks!