apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apr-util/dbm/sdbm sdbm.c sdbm_lock.c sdbm_private.h
Date Sun, 13 May 2001 10:19:44 GMT
On Wed, May 09, 2001 at 07:12:45PM -0000, wrowe@apache.org wrote:
> wrowe       01/05/09 12:12:44
> 
>   Modified:    include  apr_sdbm.h
>                dbm/sdbm sdbm.c sdbm_lock.c sdbm_private.h
>   Log:
>     Introduce refcounted apr_sdbm_[un]lock() public functions and document
>     the whole interface.

This patch and the previous looks good *except* for some very troubling
changes in apr_sdbm_firstkey()...

>...
>   --- sdbm.c	2001/05/06 13:21:17	1.20
>   +++ sdbm.c	2001/05/09 19:12:38	1.21
>...
>   @@ -412,26 +453,37 @@
>     * the following two routines will break if
>     * deletions aren't taken into account. (ndbm bug)
>     */
>   -apr_status_t apr_sdbm_firstkey(apr_sdbm_t *db, apr_sdbm_datum_t *key)
>   +APU_DECLARE(apr_status_t) apr_sdbm_firstkey(apr_sdbm_t *db, 
>   +                                            apr_sdbm_datum_t *key)
>    {
>   +    apr_status_t status;
>   +    
>   +    if ((status = apr_sdbm_lock(db, APR_FLOCK_SHARED)) != APR_SUCCESS)
>   +        return status;
>   +
>        /*
>         * start at page 0
>         */
>   -    apr_status_t status;
>   -    if ((status = read_from(db->pagf, db->pagbuf, OFF_PAG(0), PBLKSIZ))
>   -                != APR_SUCCESS)
>   -        return status;
>   +    status = read_from(db->pagf, db->pagbuf, OFF_PAG(0), PBLKSIZ);
>    
>   -    db->pagbno = 0;
>   -    db->blkptr = 0;
>   -    db->keyptr = 0;
>   +    (void) apr_sdbm_unlock(db);
>    
>        return getnext(key, db);
>    }

Why did you delete the db->pagbno line and the following two? That looks
very wrong. Those aren't the same bits as what the SDBM_INVALIDATE_CACHE
does (I'm thinking maybe you removed them thinking the invalidate would do
the same thing?)

Also, shouldn't the sequence be:

    status = getnext(key, db);
    (void) apr_sdbm_unlock(db);

?? You use that sequence in apr_sdbm_nextkey(). It also seems a bit safer to
do the getnext() within the lock.

>...
>   +APU_DECLARE(apr_status_t) apr_sdbm_lock(apr_sdbm_t *db, int type)
>...
>   +    /*
>   +     * zero size: either a fresh database, or one with a single,
>   +     * unsplit data page: dirpage is all zeros.
>   +     */

Eh? What is that comment doing there?!

>...
>   +        SDBM_INVALIDATE_CACHE(db, finfo);

Okay... I finally figured out how/why this can make sense. Basically, the
semantic is, "I just got a lock; any data that I may have had is bogus." And
since we make sure to have a lock whenever we do something, this will ensure
that stuff is tossed during the NOLOCK -> HAVELOCK transition.

Works for me :-)

Cheers,
-g

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

Mime
View raw message