From: "Greg Stein" <[EMAIL PROTECTED]>
Sent: Thursday, January 24, 2002 4:09 AM
> On Wed, Jan 23, 2002 at 06:28:06PM -0000, [EMAIL PROTECTED] 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" <[EMAIL PROTECTED]>
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, [EMAIL PROTECTED] 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
