[PATCH] lib/tdb: TDB_TRACE support (for developers)

[tridge at samba.org]

Hi Rusty,

Thanks for all the work on the tdb tracing. Some comments on the patch:

 > @@ -164,7 +164,7 @@ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int tdb_flags,
 >  	tdb->name = NULL;
 >  	tdb->map_ptr = NULL;
 >  	tdb->flags = tdb_flags;
 > -	tdb->open_flags = open_flags;
 > +	tdb->open_flags = (open_flags|TDB_NOSYNC);

Why the forcing of TDB_NOSYNC?

 > +#ifdef TDB_TRACE
 > +	{
 > +		char tracefile[strlen(name) + 32];
 > +
 > +		sprintf(tracefile, "%s.trace.%u", name, getpid());

I'd prefer use of snprintf(). The above is safe, but we'll end up getting
warnings from some loaders and static analysis tools that think that
any use of sprintf() is unsafe. Should you also cast the getpid() to
unsigned? (I wonder if pid_t is always guaranteed to be type
compatible with unsigned int, or if perhaps its 64 bit on some
platforms?)

 > +static void tdb_trace_end_ret(struct tdb_context *tdb, int ret)
 > +{
 > +	char msg[sizeof(ret) * 4];
 > +	sprintf(msg, " = %i\n", ret);
 > +	tdb_trace_write(tdb, msg);
 > +}

snprintf here too (and a few other places)

 > +void tdb_trace_2rec_flag_ret(struct tdb_context *tdb, const char *op,
 > +			     TDB_DATA rec1, TDB_DATA rec2, unsigned flag,
 > +			     int ret)
 > +{
 > +	char msg[sizeof(ret) * 4];
 > +
 > +	sprintf(msg, " %#x", flag); 

why the sizeof(ret) * 4?

Also I think we better add a test case for the "%#x" format trick in
lib/replace/snprintf.c. I don't think we've used it before, so we
better make sure our replacement snprintf supports it.

It might also be worthwhile adding a short bit of documentation
somewhere like lib/tdb/docs/tracing.txt explaining how to use tracing
(and pointing at your tracing tool).

Cheers, Tridge