You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2002/01/23 19:28:06 UTC
cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
wrowe 02/01/23 10:28:06
Modified: modules/dav/fs lock.c repos.c
Log:
Eliminate a large number of Win32-isms. In large part, these can apply
to other one-off platforms such as OS2, and immediately impact a new port
which played by all the APR rules.
Revision Changes Path
1.21 +17 -11 httpd-2.0/modules/dav/fs/lock.c
Index: lock.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/dav/fs/lock.c,v
retrieving revision 1.20
retrieving revision 1.21
diff -u -r1.20 -r1.21
--- lock.c 18 Sep 2001 03:55:41 -0000 1.20
+++ lock.c 23 Jan 2002 18:28:05 -0000 1.21
@@ -435,23 +435,28 @@
** dav_fs_build_key: Given a resource, return a apr_datum_t key
** to look up lock information for this file.
**
-** (Win32 or file is lock-null):
+** (inode/dev not supported or file is lock-null):
** apr_datum_t->dvalue = full path
**
-** (non-Win32 and file exists ):
-** apr_datum_t->dvalue = inode, dev_major, dev_minor
+** (inode/dev supported and file exists ):
+** apr_datum_t->dvalue = inode, dev
*/
static apr_datum_t dav_fs_build_key(apr_pool_t *p,
const dav_resource *resource)
{
const char *file = dav_fs_pathname(resource);
-#ifndef WIN32
apr_datum_t key;
apr_finfo_t finfo;
+ apr_status_t rv;
/* ### use lstat() ?? */
- if (apr_stat(&finfo, file, APR_FINFO_NORM, p) == APR_SUCCESS) {
-
+ /*
+ * XXX: What for platforms with no IDENT (dev/inode)?
+ */
+ rv = apr_stat(&finfo, file, APR_FINFO_IDENT, p);
+ if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
+ && ((finfo.valid & APR_FINFO_IDENT) == APR_FINFO_IDENT))
+ {
/* ### can we use a buffer for this? */
key.dsize = 1 + sizeof(finfo.inode) + sizeof(finfo.device);
key.dptr = apr_palloc(p, key.dsize);
@@ -462,7 +467,6 @@
return key;
}
-#endif
return dav_fs_build_fname_key(p, file);
}
@@ -664,9 +668,11 @@
if (*key.dptr == DAV_TYPE_FNAME) {
const char *fname = key.dptr + 1;
apr_finfo_t finfo;
+ apr_status_t rv;
/* if we don't see the file, then it's a locknull */
- if (apr_lstat(&finfo, fname, APR_FINFO_NORM, p) != APR_SUCCESS) {
+ rv = apr_lstat(&finfo, fname, APR_FINFO_MIN, p);
+ if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
if ((err = dav_fs_remove_locknull_member(p, fname, &buf)) != NULL) {
/* ### push a higher-level description? */
return err;
@@ -821,6 +827,7 @@
apr_file_t *file = NULL;
dav_error *err = NULL;
apr_size_t amt;
+ apr_status_t rv;
dav_buffer_init(p, pbuf, dirpath);
@@ -837,7 +844,8 @@
return NULL;
}
- if (apr_file_info_get(&finfo, APR_FINFO_NORM, file) != APR_SUCCESS) {
+ rv = apr_file_info_get(&finfo, APR_FINFO_NORM, file);
+ if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
err = dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
apr_psprintf(p,
"Opened but could not stat file %s",
@@ -1042,7 +1050,6 @@
return err;
}
-#ifndef WIN32
{
dav_lock_discovery *ld;
dav_lock_indirect *id;
@@ -1071,7 +1078,6 @@
return err;
}
}
-#endif
return NULL;
}
1.57 +64 -39 httpd-2.0/modules/dav/fs/repos.c
Index: repos.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/dav/fs/repos.c,v
retrieving revision 1.56
retrieving revision 1.57
diff -u -r1.56 -r1.57
--- repos.c 14 Jan 2002 13:43:24 -0000 1.56
+++ repos.c 23 Jan 2002 18:28:05 -0000 1.57
@@ -244,17 +244,43 @@
*fname_p = NULL;
}
else {
+ const char *testpath, *rootpath;
char *dirpath = ap_make_dirstr_parent(ctx->pool, ctx->pathname);
apr_size_t dirlen = strlen(dirpath);
+ apr_status_t rv = APR_SUCCESS;
- if (fname_p != NULL)
- *fname_p = ctx->pathname + dirlen;
- *dirpath_p = dirpath;
-
- /* remove trailing slash from dirpath, unless it's the root dir */
- /* ### Win32 check */
- if (dirlen > 1 && dirpath[dirlen - 1] == '/') {
- dirpath[dirlen - 1] = '\0';
+ testpath = dirpath;
+ if (dirlen > 0) {
+ rv = apr_filepath_root(&rootpath, &testpath, 0, ctx->pool);
+ }
+
+ /* remove trailing slash from dirpath, unless it's a root path
+ */
+ if ((rv == APR_SUCCESS && testpath && *testpath)
+ || rv == APR_ERELATIVE) {
+ if (dirpath[dirlen - 1] == '/') {
+ dirpath[dirlen - 1] = '\0';
+ }
+ }
+
+ /* ###: Looks like a response could be appropriate
+ *
+ * APR_SUCCESS here tells us the dir is a root
+ * APR_ERELATIVE told us we had no root (ok)
+ * APR_EINCOMPLETE an incomplete testpath told us
+ * there was no -file- name here!
+ * APR_EBADPATH or other errors tell us this file
+ * path is undecipherable
+ */
+
+ if (rv == APR_SUCCESS || rv == APR_ERELATIVE) {
+ *dirpath_p = dirpath;
+ if (fname_p != NULL)
+ *fname_p = ctx->pathname + dirlen;
+ }
+ else {
+ if (fname_p != NULL)
+ *fname_p = NULL;
}
}
}
@@ -426,7 +452,8 @@
src = apr_pstrcat(p, src_dir, "/" DAV_FS_STATE_DIR "/", src_file, NULL);
/* the source file doesn't exist */
- if (apr_stat(&src_finfo, src, APR_FINFO_NORM, p) != APR_SUCCESS) {
+ rv = apr_stat(&src_finfo, src, APR_FINFO_NORM, p);
+ if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
return NULL;
}
@@ -446,7 +473,8 @@
}
/* get info about the state directory */
- if (apr_stat(&dst_state_finfo, dst, APR_FINFO_NORM, p) != APR_SUCCESS) {
+ rv = apr_stat(&dst_state_finfo, dst, APR_FINFO_NORM, p);
+ if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
/* Ack! Where'd it go? */
/* ### use something besides 500? */
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
@@ -705,16 +733,12 @@
dav_resource_private *ctx = resource->info;
dav_resource_private *parent_ctx;
dav_resource *parent_resource;
+ apr_status_t rv;
char *dirpath;
/* If given resource is root, then there is no parent */
if (strcmp(resource->uri, "/") == 0 ||
-#ifdef WIN32
- (strlen(ctx->pathname) == 3 && ctx->pathname[1] == ':' && ctx->pathname[2] == '/')
-#else
- strcmp(ctx->pathname, "/") == 0
-#endif
- ) {
+ ap_os_is_path_absolute(ctx->pool, ctx->pathname)) {
*result_parent = NULL;
return NULL;
}
@@ -745,8 +769,9 @@
parent_resource->uri = uri;
}
- if (apr_stat(&parent_ctx->finfo, parent_ctx->pathname,
- APR_FINFO_NORM, ctx->pool) == APR_SUCCESS) {
+ apr_stat(&parent_ctx->finfo, parent_ctx->pathname,
+ APR_FINFO_NORM, ctx->pool);
+ if (rv == APR_SUCCESS || rv == APR_INCOMPLETE) {
parent_resource->exists = 1;
}
@@ -764,14 +789,13 @@
if (res1->hooks != res2->hooks)
return 0;
-#ifdef WIN32
- return stricmp(ctx1->pathname, ctx2->pathname) == 0;
-#else
- if (ctx1->finfo.filetype != 0)
+ if ((ctx1->finfo.filetype != 0) && (ctx2->finfo.filetype != 0)
+ && (ctx1->finfo.valid & ctx2->finfo.valid & APR_FINFO_INODE)) {
return ctx1->finfo.inode == ctx2->finfo.inode;
- else
+ }
+ else {
return strcmp(ctx1->pathname, ctx2->pathname) == 0;
-#endif
+ }
}
static int dav_fs_is_parent_resource(
@@ -1171,13 +1195,21 @@
else {
const char *dirpath;
apr_finfo_t finfo;
+ apr_status_t rv;
/* destination does not exist, but the parent directory should,
* so try it
*/
dirpath = ap_make_dirstr_parent(dstinfo->pool, dstinfo->pathname);
- if (apr_stat(&finfo, dirpath, APR_FINFO_NORM, dstinfo->pool) == 0
- && finfo.device == srcinfo->finfo.device) {
+ /*
+ * XXX: If missing dev ... then what test?
+ * Really need a try and failover for those platforms.
+ *
+ */
+ rv = apr_stat(&finfo, dirpath, APR_FINFO_DEV, dstinfo->pool);
+ if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
+ && (finfo.valid & srcinfo->finfo.valid & APR_FINFO_DEV)
+ && (finfo.device == srcinfo->finfo.device)) {
can_rename = 1;
}
}
@@ -1868,11 +1900,11 @@
{
const dav_liveprop_spec *info;
-#ifndef WIN32
- /* this property is not usable (writable) on the Win32 platform */
+ /* XXX this property is not usable (writable) on all platforms
+ * Need an abstract way to determine this.
+ */
if (propid == DAV_PROPID_FS_executable && !resource->collection)
return 1;
-#endif
(void) dav_get_liveprop_info(propid, &dav_fs_liveprop_group, &info);
return info->is_writable;
@@ -2036,10 +2068,11 @@
void dav_fs_gather_propsets(apr_array_header_t *uris)
{
-#ifndef WIN32
+ /* XXX: Need an abstract way to determine this on a per-filesystem basis
+ * This may change by uri (!) (think OSX HFS + unix mounts).
+ */
*(const char **)apr_array_push(uris) =
"<http://apache.org/dav/propset/fs/1>";
-#endif
}
int dav_fs_find_liveprop(const dav_resource *resource,
@@ -2077,16 +2110,8 @@
what, phdr);
(void) dav_fs_insert_prop(resource, DAV_PROPID_getetag,
what, phdr);
-
-#ifndef WIN32
- /*
- ** Note: this property is not defined on the Win32 platform.
- ** dav_fs_insert_prop() won't insert it, but we may as
- ** well not even call it.
- */
(void) dav_fs_insert_prop(resource, DAV_PROPID_FS_executable,
what, phdr);
-#endif
/* ### we know the others aren't defined as liveprops */
}
Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Jan 24, 2002 at 08:42:05AM -0600, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gs...@lyra.org>
> Sent: Thursday, January 24, 2002 4:09 AM
>...
> > Looking at the usage of 'finfo', this could be APR_FINFO_SIZE
>
> Yup, I had limited time ... so I simply made it work. That patch can definately
> be optimized based on the context of the apr_file_info_get and lstat changes!
Understood. I just happened to see it during the code review :-)
>...
> > > -#ifndef WIN32
> > > {
> > > dav_lock_discovery *ld;
> > > dav_lock_indirect *id;
> > > @@ -1071,7 +1078,6 @@
> > > return err;
> > > }
> > > }
> > > -#endif
> >
> > Hmm. Debatable change. It could end up removing a lock record, then putting
> > it right back. I don't think it is a real problem, tho.
>
> Goal 1 of APR, module code doesn't use #if PLAT fooness, or it will discover
> itself borked in future ports :) Is there a trivial patch to discover [or cache]
> the fact that the platform won't support the feature?
The WIN32 block was based on "win32 doesn't have inode/dev". When a platform
doesn't have that, then all locks are stored using pathnames. If a platform
*does* have inode/dev, then it uses them for most of the locks. However, you
can have a lock on a file that doesn't exist, so there is no inode/dev, so
those locks use the pathname. When an inode/dev arrives, then the above
#ifndef/#endif code is needed to convert a lock from the pathname style to
the inode/dev style.
So... the test is whether you have an inode/dev for a given file.
The above code removes the pathname-based lock, then computes a new key, and
saves the lock. If the platform does not support inode/dev, then the key
recomputation will come up with the same thing. No biggy. But if the
platform *does* support it, then it does the right thing.
[ as I said in the post: a bit of extra work, but it should still function
properly ]
>...
> Reviewing ... yup. This needs an apr_filepath_root() call, and test if we
> have any remaining path info [if so, it isn't the root.] apr_filepath_root
> slurps off the leading '/' in unix, the leading dir in win32/os2, and
> //mach/share on win32 (os2?) and mach:/share on netware.
>
> Correct? I'll fix.
Sounds good. One comment coming up on that patch, tho.
>...
> > If the device doesn't exist or doesn't match, then it just does a copy.
>
> My question was; can this be a tristate (can_rename, try_rename, must_copy)
> for platforms that can't tell us in advance about .dev? It's preferable
> to fail-over in the try_rename case, I would expect.
That would certainly be possible. However, once you introduce the failover,
then you wouldn't ever have can_rename. Thus, your logic would be:
* have device, if they match: try_rename
* have device, if they don't match: must_copy
* no device: try_rename
The complication arises in try_rename: how do you detect a failure due to
cross-device move/rename, as opposed to a real failure?
Maybe you just ignore most failures on a rename, and try the copy? Only give
a "real" failure if the copy fails, too?
>...
> > And one more to put back.
>
> So these work on OS2/Netware today? If so, I agree. If not, dav is broken,
> and adding a list of more platforms is the wrong solution, IMHO.
Oh, probably not :-) But we went from "working on Unix and Win32" to
"working on Unix". Thus, my complaint...
[ "working" in the sense tha the property was (correctly) present on unix,
and (correctly) absent on win32. ]
>...
> > *dirpath_p is not set in the 'else' branch.
>
> In the else case, the path is either APR_EBADPATH [meaning *dirpath_p
> isn't split into file + path, so *dirpath_p is returned to the caller
> as-given]
*dirpath_p was always set in the prior code, so "as-given" is meaningless.
> or APR_EINCOMPLETE [again *dirpath_p is returned as-was]
same.
> since the some had been too agressive in splitting off names (there
> is no meaning to //server without a successive /share/ element, together
> they form the root path).
Understood (the prior code was bad).
Just pointing out that the new code doesn't have the same invariant. That is
bad.
Ideally, the code should return an error. I can fix that...
>...
> So *dirpath_p can/should retain it's identity if ap_make_dirstr_parent really
> never had a 'child' object?
We should produce an error. That function *must* be supplied a filename. The
function says "<this> file in <that> directory." We then do some other work
in the directory.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Thursday, January 24, 2002 4:09 AM
> On Wed, Jan 23, 2002 at 06:28:06PM -0000, wrowe@apache.org wrote:
> >...
> > @@ -837,7 +844,8 @@
> > return NULL;
> > }
> >
> > - if (apr_file_info_get(&finfo, APR_FINFO_NORM, file) != APR_SUCCESS) {
> > + rv = apr_file_info_get(&finfo, APR_FINFO_NORM, file);
>
> Looking at the usage of 'finfo', this could be APR_FINFO_SIZE
Yup, I had limited time ... so I simply made it work. That patch can definately
be optimized based on the context of the apr_file_info_get and lstat changes!
> >...
> > @@ -1042,7 +1050,6 @@
> > return err;
> > }
> >
> > -#ifndef WIN32
> > {
> > dav_lock_discovery *ld;
> > dav_lock_indirect *id;
> > @@ -1071,7 +1078,6 @@
> > return err;
> > }
> > }
> > -#endif
>
> Hmm. Debatable change. It could end up removing a lock record, then putting
> it right back. I don't think it is a real problem, tho.
Goal 1 of APR, module code doesn't use #if PLAT fooness, or it will discover
itself borked in future ports :) Is there a trivial patch to discover [or cache]
the fact that the platform won't support the feature?
> >...
> > --- repos.c 14 Jan 2002 13:43:24 -0000 1.56
> > +++ repos.c 23 Jan 2002 18:28:05 -0000 1.57
> >...
> > @@ -705,16 +733,12 @@
> > dav_resource_private *ctx = resource->info;
> > dav_resource_private *parent_ctx;
> > dav_resource *parent_resource;
> > + apr_status_t rv;
> > char *dirpath;
> >
> > /* If given resource is root, then there is no parent */
> > if (strcmp(resource->uri, "/") == 0 ||
> > -#ifdef WIN32
> > - (strlen(ctx->pathname) == 3 && ctx->pathname[1] == ':' && ctx->pathname[2] == '/')
> > -#else
> > - strcmp(ctx->pathname, "/") == 0
> > -#endif
> > - ) {
> > + ap_os_is_path_absolute(ctx->pool, ctx->pathname)) {
>
> Bad and wrong.
>
> The former tested for "is this the root". ap_os_is_path_absolute() is not
> the same test. The end result is that none of the resources will have
> parents, which is going to *completely* break the lock tests.
Reviewing ... yup. This needs an apr_filepath_root() call, and test if we
have any remaining path info [if so, it isn't the root.] apr_filepath_root
slurps off the leading '/' in unix, the leading dir in win32/os2, and
//mach/share on win32 (os2?) and mach:/share on netware.
Correct? I'll fix.
> >...
> > @@ -1171,13 +1195,21 @@
> > else {
> > const char *dirpath;
> > apr_finfo_t finfo;
> > + apr_status_t rv;
> >
> > /* destination does not exist, but the parent directory should,
> > * so try it
> > */
> > dirpath = ap_make_dirstr_parent(dstinfo->pool, dstinfo->pathname);
> > - if (apr_stat(&finfo, dirpath, APR_FINFO_NORM, dstinfo->pool) == 0
> > - && finfo.device == srcinfo->finfo.device) {
> > + /*
> > + * XXX: If missing dev ... then what test?
> > + * Really need a try and failover for those platforms.
> > + *
> > + */
> > + rv = apr_stat(&finfo, dirpath, APR_FINFO_DEV, dstinfo->pool);
> > + if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
> > + && (finfo.valid & srcinfo->finfo.valid & APR_FINFO_DEV)
> > + && (finfo.device == srcinfo->finfo.device)) {
> > can_rename = 1;
>
> If the device doesn't exist or doesn't match, then it just does a copy.
My question was; can this be a tristate (can_rename, try_rename, must_copy)
for platforms that can't tell us in advance about .dev? It's preferable
to fail-over in the try_rename case, I would expect.
> >...
> > @@ -1868,11 +1900,11 @@
> > {
> > const dav_liveprop_spec *info;
> >
> > -#ifndef WIN32
> > - /* this property is not usable (writable) on the Win32 platform */
> > + /* XXX this property is not usable (writable) on all platforms
> > + * Need an abstract way to determine this.
> > + */
> > if (propid == DAV_PROPID_FS_executable && !resource->collection)
> > return 1;
> > -#endif
>
> This is a bad change. Unless/until you get the query, then you should not be
> removing this check. You're now advertising a property as being present, but
> is ineffectual.
>
> There are clients out there today which know and handle this custom
> property. You're breaking them.
I'll revert, for now.
> >...
> > @@ -2036,10 +2068,11 @@
> >
> > void dav_fs_gather_propsets(apr_array_header_t *uris)
> > {
> > -#ifndef WIN32
> > + /* XXX: Need an abstract way to determine this on a per-filesystem basis
> > + * This may change by uri (!) (think OSX HFS + unix mounts).
> > + */
> > *(const char **)apr_array_push(uris) =
> > "<http://apache.org/dav/propset/fs/1>";
> > -#endif
> > }
same here.
> >...
> > @@ -2077,16 +2110,8 @@
> > what, phdr);
> > (void) dav_fs_insert_prop(resource, DAV_PROPID_getetag,
> > what, phdr);
> > -
> > -#ifndef WIN32
> > - /*
> > - ** Note: this property is not defined on the Win32 platform.
> > - ** dav_fs_insert_prop() won't insert it, but we may as
> > - ** well not even call it.
> > - */
> > (void) dav_fs_insert_prop(resource, DAV_PROPID_FS_executable,
> > what, phdr);
> > -#endif
>
> And one more to put back.
So these work on OS2/Netware today? If so, I agree. If not, dav is broken,
and adding a list of more platforms is the wrong solution, IMHO.
From: "Greg Stein" <gs...@lyra.org>
Sent: Thursday, January 24, 2002 4:17 AM
> oops. missed this in my first response:
>
> On Wed, Jan 23, 2002 at 06:28:06PM -0000, wrowe@apache.org wrote:
> >...
> > --- repos.c 14 Jan 2002 13:43:24 -0000 1.56
> > +++ repos.c 23 Jan 2002 18:28:05 -0000 1.57
> > @@ -244,17 +244,43 @@
> > *fname_p = NULL;
> > }
> > else {
> > + const char *testpath, *rootpath;
> > char *dirpath = ap_make_dirstr_parent(ctx->pool, ctx->pathname);
> > apr_size_t dirlen = strlen(dirpath);
> > + apr_status_t rv = APR_SUCCESS;
> >
> > - if (fname_p != NULL)
> > - *fname_p = ctx->pathname + dirlen;
> > - *dirpath_p = dirpath;
>
> Note that *dirpath_p was _always_ set.
Yes...
> > -
> > - /* remove trailing slash from dirpath, unless it's the root dir */
> > - /* ### Win32 check */
> > - if (dirlen > 1 && dirpath[dirlen - 1] == '/') {
> > - dirpath[dirlen - 1] = '\0';
> > + testpath = dirpath;
> > + if (dirlen > 0) {
> > + rv = apr_filepath_root(&rootpath, &testpath, 0, ctx->pool);
> > + }
> > +
> > + /* remove trailing slash from dirpath, unless it's a root path
> > + */
> > + if ((rv == APR_SUCCESS && testpath && *testpath)
> > + || rv == APR_ERELATIVE) {
> > + if (dirpath[dirlen - 1] == '/') {
> > + dirpath[dirlen - 1] = '\0';
> > + }
> > + }
> > +
> > + /* ###: Looks like a response could be appropriate
> > + *
> > + * APR_SUCCESS here tells us the dir is a root
> > + * APR_ERELATIVE told us we had no root (ok)
> > + * APR_EINCOMPLETE an incomplete testpath told us
> > + * there was no -file- name here!
> > + * APR_EBADPATH or other errors tell us this file
> > + * path is undecipherable
> > + */
> > +
> > + if (rv == APR_SUCCESS || rv == APR_ERELATIVE) {
> > + *dirpath_p = dirpath;
> > + if (fname_p != NULL)
> > + *fname_p = ctx->pathname + dirlen;
> > + }
> > + else {
> > + if (fname_p != NULL)
> > + *fname_p = NULL;
> > }
>
> *dirpath_p is not set in the 'else' branch.
In the else case, the path is either APR_EBADPATH [meaning *dirpath_p
isn't split into file + path, so *dirpath_p is returned to the caller
as-given] or APR_EINCOMPLETE [again *dirpath_p is returned as-was]
since the some had been too agressive in splitting off names (there
is no meaning to //server without a successive /share/ element, together
they form the root path).
> Also note that no callers expect those values to be set to NULL. Given that
> the paths have been processed by Apache already, it's a good bet they are
> always valid.
Correct - this is a pendantic case, and is definately tied to the earlier
error I introduced above testing os_is_path_absolute, when it should have
mirrored this test.
So *dirpath_p can/should retain it's identity if ap_make_dirstr_parent really
never had a 'child' object?
Bill
Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Jan 23, 2002 at 06:28:06PM -0000, wrowe@apache.org wrote:
>...
> @@ -837,7 +844,8 @@
> return NULL;
> }
>
> - if (apr_file_info_get(&finfo, APR_FINFO_NORM, file) != APR_SUCCESS) {
> + rv = apr_file_info_get(&finfo, APR_FINFO_NORM, file);
Looking at the usage of 'finfo', this could be APR_FINFO_SIZE
>...
> @@ -1042,7 +1050,6 @@
> return err;
> }
>
> -#ifndef WIN32
> {
> dav_lock_discovery *ld;
> dav_lock_indirect *id;
> @@ -1071,7 +1078,6 @@
> return err;
> }
> }
> -#endif
Hmm. Debatable change. It could end up removing a lock record, then putting
it right back. I don't think it is a real problem, tho.
>...
> --- repos.c 14 Jan 2002 13:43:24 -0000 1.56
> +++ repos.c 23 Jan 2002 18:28:05 -0000 1.57
>...
> @@ -705,16 +733,12 @@
> dav_resource_private *ctx = resource->info;
> dav_resource_private *parent_ctx;
> dav_resource *parent_resource;
> + apr_status_t rv;
> char *dirpath;
>
> /* If given resource is root, then there is no parent */
> if (strcmp(resource->uri, "/") == 0 ||
> -#ifdef WIN32
> - (strlen(ctx->pathname) == 3 && ctx->pathname[1] == ':' && ctx->pathname[2] == '/')
> -#else
> - strcmp(ctx->pathname, "/") == 0
> -#endif
> - ) {
> + ap_os_is_path_absolute(ctx->pool, ctx->pathname)) {
Bad and wrong.
The former tested for "is this the root". ap_os_is_path_absolute() is not
the same test. The end result is that none of the resources will have
parents, which is going to *completely* break the lock tests.
>...
> @@ -1171,13 +1195,21 @@
> else {
> const char *dirpath;
> apr_finfo_t finfo;
> + apr_status_t rv;
>
> /* destination does not exist, but the parent directory should,
> * so try it
> */
> dirpath = ap_make_dirstr_parent(dstinfo->pool, dstinfo->pathname);
> - if (apr_stat(&finfo, dirpath, APR_FINFO_NORM, dstinfo->pool) == 0
> - && finfo.device == srcinfo->finfo.device) {
> + /*
> + * XXX: If missing dev ... then what test?
> + * Really need a try and failover for those platforms.
> + *
> + */
> + rv = apr_stat(&finfo, dirpath, APR_FINFO_DEV, dstinfo->pool);
> + if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
> + && (finfo.valid & srcinfo->finfo.valid & APR_FINFO_DEV)
> + && (finfo.device == srcinfo->finfo.device)) {
> can_rename = 1;
If the device doesn't exist or doesn't match, then it just does a copy.
>...
> @@ -1868,11 +1900,11 @@
> {
> const dav_liveprop_spec *info;
>
> -#ifndef WIN32
> - /* this property is not usable (writable) on the Win32 platform */
> + /* XXX this property is not usable (writable) on all platforms
> + * Need an abstract way to determine this.
> + */
> if (propid == DAV_PROPID_FS_executable && !resource->collection)
> return 1;
> -#endif
This is a bad change. Unless/until you get the query, then you should not be
removing this check. You're now advertising a property as being present, but
is ineffectual.
There are clients out there today which know and handle this custom
property. You're breaking them.
>...
> @@ -2036,10 +2068,11 @@
>
> void dav_fs_gather_propsets(apr_array_header_t *uris)
> {
> -#ifndef WIN32
> + /* XXX: Need an abstract way to determine this on a per-filesystem basis
> + * This may change by uri (!) (think OSX HFS + unix mounts).
> + */
> *(const char **)apr_array_push(uris) =
> "<http://apache.org/dav/propset/fs/1>";
> -#endif
> }
Same problem. The #ifndef should be restored unless/until you have the query
mechanism in place.
>...
> @@ -2077,16 +2110,8 @@
> what, phdr);
> (void) dav_fs_insert_prop(resource, DAV_PROPID_getetag,
> what, phdr);
> -
> -#ifndef WIN32
> - /*
> - ** Note: this property is not defined on the Win32 platform.
> - ** dav_fs_insert_prop() won't insert it, but we may as
> - ** well not even call it.
> - */
> (void) dav_fs_insert_prop(resource, DAV_PROPID_FS_executable,
> what, phdr);
> -#endif
And one more to put back.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
another one... Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
Posted by Greg Stein <gs...@lyra.org>.
oops. missed this in my first response:
On Wed, Jan 23, 2002 at 06:28:06PM -0000, wrowe@apache.org wrote:
>...
> --- repos.c 14 Jan 2002 13:43:24 -0000 1.56
> +++ repos.c 23 Jan 2002 18:28:05 -0000 1.57
> @@ -244,17 +244,43 @@
> *fname_p = NULL;
> }
> else {
> + const char *testpath, *rootpath;
> char *dirpath = ap_make_dirstr_parent(ctx->pool, ctx->pathname);
> apr_size_t dirlen = strlen(dirpath);
> + apr_status_t rv = APR_SUCCESS;
>
> - if (fname_p != NULL)
> - *fname_p = ctx->pathname + dirlen;
> - *dirpath_p = dirpath;
Note that *dirpath_p was _always_ set.
> -
> - /* remove trailing slash from dirpath, unless it's the root dir */
> - /* ### Win32 check */
> - if (dirlen > 1 && dirpath[dirlen - 1] == '/') {
> - dirpath[dirlen - 1] = '\0';
> + testpath = dirpath;
> + if (dirlen > 0) {
> + rv = apr_filepath_root(&rootpath, &testpath, 0, ctx->pool);
> + }
> +
> + /* remove trailing slash from dirpath, unless it's a root path
> + */
> + if ((rv == APR_SUCCESS && testpath && *testpath)
> + || rv == APR_ERELATIVE) {
> + if (dirpath[dirlen - 1] == '/') {
> + dirpath[dirlen - 1] = '\0';
> + }
> + }
> +
> + /* ###: Looks like a response could be appropriate
> + *
> + * APR_SUCCESS here tells us the dir is a root
> + * APR_ERELATIVE told us we had no root (ok)
> + * APR_EINCOMPLETE an incomplete testpath told us
> + * there was no -file- name here!
> + * APR_EBADPATH or other errors tell us this file
> + * path is undecipherable
> + */
> +
> + if (rv == APR_SUCCESS || rv == APR_ERELATIVE) {
> + *dirpath_p = dirpath;
> + if (fname_p != NULL)
> + *fname_p = ctx->pathname + dirlen;
> + }
> + else {
> + if (fname_p != NULL)
> + *fname_p = NULL;
> }
*dirpath_p is not set in the 'else' branch.
Also note that no callers expect those values to be set to NULL. Given that
the paths have been processed by Apache already, it's a good bet they are
always valid.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/