apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <ad...@rowe-clan.net>
Subject Re: cvs commit: apr-util/dbm/sdbm sdbm.c
Date Tue, 01 May 2001 22:35:47 GMT
From: "Greg Stein" <gstein@lyra.org>
Sent: Tuesday, May 01, 2001 12:45 AM


> On Mon, Apr 30, 2001 at 09:50:25PM -0500, William A. Rowe, Jr. wrote:
> > From: "Greg Stein" <gstein@lyra.org>
> > Sent: Monday, April 30, 2001 8:49 PM
> > 
> > > -1. Please back out.
> > 
> > I'd rather push forward and address your other concerns...
> 
> Sorry. In this case, that would not be prudent. Please back out.
> 
> The situation is that the SDBM is ten *years* old. It doesn't have bugs. It
> simply is not something that I worry about. These kinds of changes are
> destabilizing, in an area where I think we can't afford it.

Greg, now you have me confused (unless I've missed a post on this subject !?!)

I noted your commits (all look good) and have some additional thoughts below,
but the question in my mind is ... do you want me to unroll these patches?

> >...
> > I'm well aware of this.  That's why I posited the question to the list about
> > the cache invalidation semantic.  The other option, of course, is to drop cache

> > entirely in a shared open.
> 
> The latter is the only option (short of a synchronized / distributed cache
> thingameebob). The mtime isn't going to be fine-grained enough to give you
> proper invalidation. Two changes within the same second will cause data
> corruption.

I also realized that not all Win based systems can be trusted to even update the
mtime upon every update.  I only see one answer;

  * expose lock/unlock for multiple operations that take advantage of the cache
  * flush the cache upon granting every lock
  * be sure we aren't using buffered writes on either the pagf or dirf

This is certainly sub-optimal, but still significantly faster than closing and
opening the two files for every request.

> > Please note we continue to lock the file for it's lifetime iff the user doesn't
> > pass the new APR_SHARELOCK flag.  See the snippets I grabbed from the commit 
> > log below.
> 
> Sure, but I believe the existing change needs to be reverted, then we can
> move forward in *teeny* pieces. I *really* am hesitant to get near this
> code. If we change it, then we should only do so in a narrow, limited
> fashion.

Be happy to, see my earlier comments, but your patches futher confused me.

> As a starter, the error handling needs to be fixed. That whole
> apr_sdbm_error_get() isn't needed now that you've changed everything to
> return a status. That will clear up the ioerr() macros, and the numerous
> "goto error" lines. As a result, that will simplify error recovery in the
> functions, which is very important to ensure that we unlock when necessary.
>
> Then we need the function for cache invalidation, which you have in the
> posted patch.
> 
> Lastly, we would have a function/macro to lock on entry (if needed), and
> then another to invalidate/unlock on exit. The mechanism for inval/unlock
> needs to be tightly bound.

Good patches, as I said.  I need to vet this code too, and I'll have more
comments in a couple of hours, and add the invalidation (not optimized by
mtime stamps) at the same time.

> > > I am mildly against the concept, in general, for SDBM (since people can turn
> > > to other DBM implementations), but if you come up with a patch that solves
> > > the issues, then we could certainly discuss it.
> > 
> > I'm not proposing we provide the most optimized solution in the world.
> 
> Understood. Invalidating the cache would be an adequate solution, and I'm
> okay with that. But the patch needs to be reverted, then we can start with
> small bits to ensure absolute correctness.
> 
> Example? I found a bug in your latest addition. A failure in chkpage() could
> have resulted in returning APR_SUCCESS.

Thanks for that patch.

> (this was the checkin *before* the locking changes; I haven't reviewed the
>  locking change because of the glaring cache issue; who knows what is there?
>  I don't want to have to worry about it)
> 
> >...
> > > There are some other issues with this patch, but it's kind of moot...
> > 
> > Bring them on.  Again, I prefer to work forward, since I mentioned in my other
> 
> Sorry, but we'll revert and start over. Get the fundamentals proper, then
> add in the bits we need. With this code, that is much better than jumping
> ten steps and hope that we get the previous nine filled back in.

Again, your patches confuse me, please clarify.

> > comment, the auth_dbm, auth_digest, and soon ssl will need at least a bare-bones
> > cross-process dbm.
> 
> sdbm is already cross-process. It arbitrates access to the DBM quite fine.
> You're simply trying to make the locking more fine-grained. That's okay, but
> not at the expense of trust in this code.

Huh?  By closing and reopening the file (exclusively)?  I don't follow.

> > I'd prefer to dump caching entirely for APR_SHARELOCK access, rather than revert
> > to the prior code, but if we can find a better way I'd like that.  Dumping caching
> > can't be worse than closing up and reopening an sdbm on every request.
> 
> Dumping the cache will be a *lot* better than close/reopen.

Agreed!


Mime
View raw message