apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [proposal] DBM - allow multiple DBM's of differnt types at the same time.
Date Wed, 22 Aug 2001 09:54:05 GMT
-1 until you put that GDBM support back in. It isn't right to just drop it.

On Mon, Aug 20, 2001 at 04:50:25PM -0700, Ian Holsman wrote:
> On Mon, 2001-08-20 at 13:07, Sterling Hughes wrote:
>...
> > 	Not sure I understand what you're saying?  There are ways to allow
> > 	the registering of module functionality, without modifying the main
> > 	package.  Just provide a function like:
> > 
> > 	apr_dbm_register()
> > 
> > 	Which registers a dbm internally, and then have a constant defined
> > 	in a seperate file, and all is clear...
> that would work,
> but that wouldn't that mean there would be need to some init code
> somewhere to call the registration function?

Every time we load a module, we call registration functions. That is no big
deal.

Your solution of apr_dbm_*_open() still requires a recompilation/relink. The
registration mechanism is completely open-ended.

> > > 2. I may have a dbm which needs extra parameters to open it. (say for
> > > exampe cache size, or maybe it requires
> > >     a fixed key length defined) I couldn't do this simply
> > >
> > 
> > 	dbm_set_x() and check the flags, once they're all filled out (a
> > 	va_args implementation could also be done...)

Not sure what you mean by dbm_set_x() here. You have to open the thing first
to get an apr_dbm_t. But then you're too late.

The va_arg solution is a semi-reasonable solution, but that kills any chance
for type checking.

> > 	Furthermore, the idea of an abstraction layer is too bring all these
> > 	down to a lowest common denominator, at the cost of functionality
> > 	sometimes; usually functions that take extra arguments, can be
> > 	filled in with acceptable defaults.  You'll run into the same
> > 	problems in that some db's don't support certain features, and
> > 	others do, the idea is to concentrate on the functionality that is
> > 	necessary and shared between the different db's (wrap this in a
> > 	large imho :).

Exactly. apr_dbm exposes DBM-style functionality. Simple key/value pairs.
That is it. The open functionality does not need flexibility. It has not
been demonstrated yet that we need more capability there.

> true, thats why they all return the same thing (apr_dbm_t)
> the open_DBM function was supposed to be called when the devloper wanted
> to do something specific with this type of dbm. so in the berkley DB
> case there could be a apr_dbm_db_getRaw() function which returns the DB*
> so that developers are not constrained the set of functions we chose to
> abstract.

No... we already have patterns for getting at underlying data types. See
apr_portable.h. Follow that pattern *IF* we are to expose those. IMO, we
should not do so.

> (BTW I was planning on implmenting a shared-memory hash table, that I
> posted ages ago, via this DBM interface, and it required a different set
> of opening parameters (key/elem size, #elements) these parameters could
> be passed via a db_set function call but it would look clunky

No... keys and values are arbitrary length, as specified by an apr_datum_t.
If your hash cannot handle that, then it cannot be used in the APR DBM
interface.

>...
> so..
> shall we put it to a vote?
> 
> apr_dbm_open_XXX
> or 
> apr_dbm_open(XXX)

Registration and the second form.


But your patch still has lots of work anyways. The GDBM functionality can't
just disappear. Sorry, but the DAV functionality uses apr_dbm and you'd
completely break access to the property files.

Second, the vtable should not be part of the runtime apr_dbm_t structure.
You should have a pointer to a static const table, like we do with buckets.

The formatting may have gone thru "indent" but it still sucks. Just a glance
over it shows some random gunk. For example, what the heck is up with the
formatting of apr_datum_t in apr_dbm.h? And all the spacing of the function
params in apr_dbm.h has been changed. There are extra blank lines at the end
of some functions (after returns), and some missing blank lines between
functions.

We should also see a patch against apr_dbm.h, rather than the whole file. I
can't see if you've changed the public interface or not.

dbm_private.h is missing a license and disclaimer. It also has a
non-standard #ifndef/#define/#endif pattern around it. We never start the
symbol with "_", and on all the new .h files you specified: toss the "1"
from the #define ... we don't do that either.

Why is the "used names" functionality part of the per-db public interface?
That should be part of the hook stuff just like the rest.

In any case... the per-db headers should go, in favor of the registration
mechanism. You would register each DB style with a name. You can then open a
DB style by name, or using the reserved "any". Note that we would define
some standard names:

#define APR_DBM_USE_SDBM "sdbm"
#define APR_DBM_USE_GDBM "gdbm"
#define APR_DBM_USE_DB   "db"
#define APR_DBM_USE_ANY  "any"

Also, the whole point of this exercise was to link in all of the variations.
That means the .c files should not have the #if APU_USE_*/#endif stuff
around them. The APU_USE_* symbols are only used to determine which DB to
use for the "any" case.


So... -1 for now. The patch still needs work before it can be applied.

Cheers,
-g

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

Mime
View raw message