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

[Rusty Russell]

On Fri, 28 Aug 2009 12:57:34 pm tridge at samba.org wrote:
>  > -	tdb->open_flags = open_flags;
>  > +	tdb->open_flags = (open_flags|TDB_NOSYNC);
> 
> Why the forcing of TDB_NOSYNC?

Oops.  Fixed.

>  > +#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.

How disappointing.  If the compiler got clever enough to tell if such a thing
really would overflow, we'd want to know.  With snprintf, we won't ever know :(

> 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?)

POSIX says it's a signed integer type.  I'll use (long) to be safe.

snprintf fixed elsewhere, too.  I note your always nul-terms, good!

>  > +			     int ret)
>  > +{
>  > +	char msg[sizeof(ret) * 4];
>  > +
>  > +	sprintf(msg, " %#x", flag); 
> 
> why the sizeof(ret) * 4?

stdrusty.h has:

/* Upper bound to sprintf this simple type?  Each 3 bits < 1 digit. */
#define CHAR_SIZE(type) (((sizeof(type)*CHAR_BIT + 2) / 3) + 1)

I didn't find an equiv.  So *4 seemed like a reasonable simplification.

> 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's a silly extension anyway.  I'll just use 0x%x.

> 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).

Yep, good idea.

Patch coming...
Rusty.