tridge at tridge at
Mon Feb 19 04:42:44 GMT 2007


 > Attached find a patch that adds tdb_parse_record():

I think this is a great idea!

A couple of minor nits:

 > +int tdb_parse_record(struct tdb_context *tdb, TDB_DATA key,
 > +		     int (*parser)(TDB_DATA data, void *private_data),
 > +		     void *private_data);

Maybe pass the key into the callback as well? I know its not strictly
necessary, but I think it would be useful.

 > +	if ((tdb->transaction == NULL) && (tdb->map_ptr != NULL)) {
 > +		/*
 > +		 * Optimize by avoiding the malloc/memcpy/free, point the
 > +		 * parser directly at the mmap area.
 > +		 */
 > +		data.dptr = offset + (char *)tdb->map_ptr;
 > +		return parser(data, private_data);
 > +	}

I think you need a call to tdb_oob() here, in case the tdb has grown
and this instances mmap size is not the full file. The tdb_oob() call
will grow the mmap if needed.

There is also a slight danger that the callback function could call
another tdb_*() function which in turn calls tdb_oob() and changes the
mmap base pointer, which could cause the callback to crash. Probably
best to cope with this in documentation by just saying that you may
not call any other tdb functions inside the callback (or that any
further calls can invalidate the data).

I'm not so keen on this code:

 > +	if (rec_ptr == 0) {
 > +		data.dptr = NULL;
 > +		data.dsize = 0;
 > +		return parser(data, private_data);
 > +	}

why not just use 

instead of calling the callback with null data? Otherwise how would
you differentiate between an empty record and a non-existant record?

Cheers, Tridge

More information about the samba-technical mailing list