tdb_chainlock() in tdb1, tdb2 and tdb_compat ?

[Jeremy Allison]

On Thu, Apr 12, 2012 at 04:58:30PM +0200, Volker Lendecke wrote:
> Hi!
> 
> Can someone on this list enlighten me? I am confused about
> the role of tdb_compat. In the combined build, what happens
> in legacy source3 code if I call tdb_chainlock? When using
> the autoconf based build, I think I should get tdb1 with the
> calling convention from lib/tdb/include/tdb.h:
> 
> int tdb_chainlock(struct tdb_context *tdb, TDB_DATA key);
> 
> This returns 0 on success and -1 (and only -1) on error.
> When running the combined toplevel build with tdb2, I get
> the impression that we end up calling tdb_chainlock from
> lib/tdb2/tdb2.h:
> 
> enum TDB_ERROR tdb_chainlock(struct tdb_context *tdb, TDB_DATA key);
> 
> This has a similar calling convention that also compiles
> fine in source3, but with an implicit enum->int conversion.
> The slight problem I see (assuming we do call tdb_chainlock
> returning the enum) is that TDB_ERROR not only is 0 for
> success and -1 for failure, but also uses other negative
> values:
> 
> enum TDB_ERROR {
>         TDB_SUCCESS     = 0,    /* No error. */
>         TDB_ERR_CORRUPT = -1,   /* We read the db, and it was bogus. */
>         TDB_ERR_IO      = -2,   /* We couldn't read/write the db. */
>         TDB_ERR_LOCK    = -3,   /* Locking failed. */
>         TDB_ERR_OOM     = -4,   /* Out of Memory. */
>         TDB_ERR_EXISTS  = -5,   /* The key already exists.  */
>         TDB_ERR_NOEXIST = -6,   /* The key does not exist.  */
>         TDB_ERR_EINVAL  = -7,   /* You're using it wrong. */
>         TDB_ERR_RDONLY  = -8,   /* The database is read-only. */
>         TDB_ERR_LAST = TDB_ERR_RDONLY
> };
> 
> So hypothetical callers checking
> 
> if (tdb_chainlock(...) == -1) {
> 	/* error path */
> }
> 
> silently would become wrong.

Ouch. This is a *BIG* API problem (IMHO). Any tdb_compat()
layer has to provide *exact* tdb1 semantics or we're setting
ourselves up for a world of pain with different semantics
called from code that doesn't expect it.

Jeremy.