[PATCH] lib/tdb: locking cleanup

[Rusty Russell]

On Fri, 28 Aug 2009 01:25:47 pm tridge at samba.org wrote:
> Hi Rusty,
> 
>  > -int tdb_brlock(struct tdb_context *tdb, tdb_off_t offset, 
>  > -	       int rw_type, int lck_type, int probe, size_t len)
>  > +int tdb_brlock(struct tdb_context *tdb,
>  > +	       int rw_type, tdb_off_t offset, size_t len,
>  > +	       enum tdb_lock_flags flags)
>  >  {
>  > -	struct flock fl;
>  >  	int ret;
>  >  
>  >  	if (tdb->flags & TDB_NOLOCK) {
>  >  		return 0;
>  >  	}
>  >  
>  > +	if (flags & TDB_LOCK_MARK_ONLY) {
>  > +		return 0;
>  > +	}
> 
> Using an enum as a bitmask seems a bit strange. It's valid C, but
> anyone reading it will think that the enum takes on the set of values
> listed in the enum declaration, and may do something like a switch
> statement with the various cases. Why use an enum when you mean it to
> be masked like this?

Habit, probably.  I was originally going to ditch "mark" altogether, but
that seems a while off.

I'll change to #defines.

> btw, the only user I know of the 'mark' locks is ctdb. Have you or
> Ronnie tested this patch with ctdb?

No.  Which brings us to a discussion of unit tests.  I have some unit tests
for tdb (in ccan), and I'd really like to have them in samba, esp. for the lib
dir.  Higher-level testing is great, but unit tests are better for isolating
bugs and testing nasty corners.

The ccan unit tests sit in a test/ operate as so:
1) C files starting with "api" are compiled and linked against the library,
   then executed.  These should work on all future versions of the lib.
2) C files starting with "run" are compiled and run (usually, they explicitly
   #include the C files from .. so they can test static funcs and override
   internal stuff.  Works really well in practice).
3) C files starting with "compile_ok" or "compile_fail" are simply compiled
   and linked.  Not sure we need these.
4) All other C files are linked into everything, for helper routines.

I use libtap, which creates simple one-line-per-test output (the TAP output
format is standardized see testanything.org)

If people think this is a good idea, I'm happy to whip up a patch...

Thanks!
Rusty.