apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject RE: apr_fileinfo_t is ___BOGUS___
Date Thu, 11 Jan 2001 10:18:35 GMT
On Wed, Jan 10, 2001 at 09:43:16PM -0800, Greg Stein wrote:
>...
> ----- Forwarded message from "William A. Rowe, Jr." <wrowe@rowe-clan.net> -----
>...
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: Wednesday, January 10, 2001 6:35 PM
>...
> > On Wed, Jan 10, 2001 at 02:21:37PM -0600, William A. Rowe, Jr. wrote:
> > > Ok guys...
> > > 
> > >   apr_fileinfo_t must be restored to an incomplete type.
> > > 
> > > On win32, we have the following scenario;
> > > 
> > >   inode, dev, and ctype require a CreateFile call.
> > 
> > ctype?
> 
> file, pipe, char, dev etc.  Simple typo.

Ah. Actually, it is "filetype". I just did a search, and found that Apache
only cares about the following types: APR_NOFILE, APR_REG, APR_DIR, and
APR_LNK.

> > >   type (pipe, etc) requires CreateFile/GetFileType calls.
> > >   query owner/access kills performance by two orders of magnitude.
> > 
> > that doesn't happen today. user/group fields are zeroed.
> 
> Ack.  I certainly expect to see an accessor for these where they are
> -required-, e.g. permission properties of dav?

DAV certainly doesn't need those. Looks like mod_include and mod_rewrire are
the only users for those two fields.

> 
> > >   truename requires a FindFile call.
> > 
> > truename? eh?
> 
> eh, ya, eh.  We may not need it everywhere, but we need it some places.

My point was: what the heck are you talking about? There is no "truename"
field anywhere in the apr_fileinfo_t structure. Since it isn't there, you
can't complain about it :-)

>...
> > > Now, there is no SINGLE ATOMIC CALL that does all this work for
> > > us.  It -should- be our problem (the win32 hackers) to get this
> > > right, but we can only do that with accessor functions.
> > 
> > You're talking about performance, not correctness... right?
> 
> It's (nearly) correct now,

If there are correctness issues, then they should be fixed regardless.

> but as Stoddard observed, performancewise
> we have work to do.

Okay. Just wanted to be sure that you're only talking about perf here.

And if we're talking about perf changes as the cause for an API change, then
I'd like to see some before/after numbers to really know what we're talking
about here. Turning it into an opaque type for a 5% win is a poor tradeoff
in my book :-)

> > > The changes need to also augment the lstat flavor, so that the
> > > one can be queried from the other (a significant performance
> > > boost on win32.)
> > 
> > I don't understand this "queried from the other" bit.
> 
> If we have resolved the truename of the link file, do we really need
> to call findfirstfile again as the target and get a truename again?  No.

That truename stuff again. From your earlier discussion, that sounds like
some of the canonical name / trusted name stuff. That doesn't belong in this
discussion, does it? I don't see a truename in apr_fileinfo_t, and I don't
see that it would be added; so that point seems moot.

> > > BTW - we also need an optimized form of apr_file_open that accepts
> > > an apr_fileinfo_t ... that handle we grabbed to stat the file can
> > > be Duplicated into a read or read/write handle for file access!  The
> > > file handle in the win32 flavor will be cached with a registered
> > > cleanup against the pool, so until we are destroyed, we can keep
> > > grabbing the file for different flavors of access.
> > 
> > That is a bit of a stretch. How about saying people can open the file, then
> > use apr_getfileinfo() on it. (as opposed to supporting the other direction)
> 
> Both.  Either way, we have a handle embedded in both structures.  If it's
> there, we can DuplicateHandle on win32.

Eh? Embedded in both structures? No... I'm saying only in the apr_file_t. If
a person wants to open the file just once, then they should open the darn
thing with apr_open(), then use apr_getfileinfo() on the result.

apr_getfileinfo() would just use the already-opened handle from the
apr_file_t structure. No double handles, no re-opening.

>...
> > *) the code needs refactoring big time (reduce code dup errors).
> 
> Ack.  Already once-throughed and reduced utf folding to four blocks, and
> even that can be compressed.  Other opportunities there.

I see two calls to utf8_to_unicode_path(). It really sounds like you're
working with a totally different filestat.c. We can't effectively talk about
revamping the API when you're working with something else.

> > *) in apr_stat(): if the CreateFileW() fails because the user doesn't have
> >    permission to open the file, then the code should fall thru to the
> >    GetFileAttributesEx() path.
> 
> Cool thought!

Credit Bill Tutt on this one. He ran into it.

[ Bill actually had a few of these suggestions, and I looked through to see
  what he was talking about. eek. ]

>...
> > *) refactoring would prevent the fake construction of an incomplete
> >    apr_file_t (which could pose maintenance problems if apr_getfileinfo()
> >    assumed that another field was valid, but the fake ones didn't set it
> >    up). the refactoring would just pass a filehandle into an
> >    attribute-fetching funtcion, and apr_getfileinfo() would also call this
> >    function.
> 
> Actually, based on the volume and other factors, several fields are invalid
> on a runtime basis (CreateFileW fell through?  No inode, it's APR_ENOTIMPL.
> Same always on Win9x.)

Sure, but that doesn't obviate the need to simplify the code.


When we get some perf numbers, then we can look at the root cause of the
problem here. I'm thinking that rather than make the whole thing opaque,
that we just move key (slow) items to other functions. But then we bung up
Unix systems where this falls out in a single step. Hard to say, which is
why we need perf data.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message