On Thu, Jan 24, 2002 at 08:42:05AM -0600, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <[EMAIL PROTECTED]>
> 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/
