apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] apr_dbm -- db fix
Date Tue, 18 Sep 2001 10:43:35 GMT
On Tue, Sep 18, 2001 at 10:05:07AM -0700, Mladen Turk wrote:
> > -----Original Message-----
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: 17. rujan 2001 13:57
> > To: Mladen Turk
> > Cc: APR Dev List
> > Subject: Re: [PATCH] apr_dbm -- db fix
> > 
> > -1 -- this patch should not be applied.
> > 
> > The change to RETURN_DATUM is wrong. The result of the NEXTKEY is placed
> > into "rd", and that is returned. If DB's do_nextkey is putting the result
> > into *pkey rather than *pnext, then *that* is the bug. But changing the
> > RETURN_DATUM call is wrong.
> > 
> > I *know* that the apr_dbm_nextkey() function is working. If DB isn't, then
> > you should question the macros instead.
> > 
> [Mladen Turk] 
> Sorry but did you try to run the testdbm utility?

With the SDBM database... yup. All the time.

That was my point -- the code dealing with RETURN_DATUM is correct. The
whole point of "result_datum_t" and the "rd" variable is to *return* values.
Thus, the change to return a value via ckey is just wrong.

> I've tested it using db-1.85 and db-3.3.11
> 1. Insert some values using 'testdbm build whatever'
> 2. Run 'testdbm list whatever'
> 3. After the first record read you'll get a segfault

Fine. That just says the DB portion is wrong, and do_nextkey() needs to be
fixed. The apr_dbm_nextkey() function shouldn't change.

>...
> Now, when I apply the patch, on db-3.3.11 I have a infinite loop. The
> reason for that is that data DBT must be memset to zero.
>...
> @@ -255,8 +255,13 @@
>      int dberr;
>      DBT data;
>  
> +    memset(&data,0,sizeof(DBT));

It is easier and faster to do:

    DBT data = { 0 };

>  #if DB_VER == 1
>      dberr = (*f->bdb->seq)(f->bdb, pkey, &data, R_NEXT);
> +    if (dberr == RET_SPECIAL) {
> +        pkey->data = NULL;
> +        pkey->size = 0;
> +    }

This should be setting pnext to NULL/0.

Looking at that function... yes, it is quite broken. It doesn't do anything
with pnext. It should. Notice that the other NEXTKEY macros all set the
"next key" parameter, rather than modifying the key in place.

Thus: my veto stands against any modification to apr_dbm_nextkey(). Please
make your fixes in do_nextkey().

Cheers,
-g

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

Mime
View raw message