[PATCHES] Cleanup dead records in messages.tdb

[Christof Schmitt]

On Thu, Apr 03, 2014 at 02:56:39PM -0700, Jeremy Allison wrote:
> On Thu, Apr 03, 2014 at 02:32:25PM -0700, Jeremy Allison wrote:
> > On Thu, Apr 03, 2014 at 02:26:29PM -0700, Christof Schmitt wrote:
> > > 
> > > Actually, there is an issue with the locking here: tdb_delete fails,
> > > because it tries to get a lock for the delete, but the chain is already
> > > locked. Either we have to live with the race here, or add a call to tdb
> > > that allows deletion without acquiring the lock.
> > 
> > Are you sure here ? That same bug would then exist
> > in the function :
> > 
> > tatic NTSTATUS retrieve_all_messages(TDB_CONTEXT *msg_tdb,
> >                                       struct server_id id,
> >                                       TALLOC_CTX *mem_ctx,
> >                                       struct messaging_array **presult)
> > {
> >         struct messaging_array *result;
> >         TDB_DATA key = message_key_pid(mem_ctx, id);
> >         NTSTATUS status;
> > 
> >         if (tdb_chainlock(msg_tdb, key) != 0) {
> >                 TALLOC_FREE(key.dptr);
> >                 return NT_STATUS_LOCK_NOT_GRANTED;
> >         }
> > 
> >         status = messaging_tdb_fetch(msg_tdb, key, mem_ctx, &result);
> > 
> >         /*
> >          * We delete the record here, tdb_set_max_dead keeps it around
> >          */
> >         tdb_delete(msg_tdb, key);
> >         tdb_chainunlock(msg_tdb, key);
> > 
> >         if (NT_STATUS_IS_OK(status)) {
> >                 *presult = result;
> >         }
> > 
> >         TALLOC_FREE(key.dptr);
> > 
> >         return status;
> > }
> > 
> > which is called regularly in the messaging
> > code...
> 
> Just wrote a quick standalone test:
> 
> +static void jratest(TDB_CONTEXT *db)
> +{
> +       TDB_DATA key, data;
> +
> +       key.dptr = "keyname";
> +       key.dsize = 8;
> +       data.dptr = "data";
> +       data.dsize = 5;
> +
> +       if (tdb_store(db, key, data, 0) != 0) {
> +               abort();
> +       }
> +
> +       if (tdb_chainlock(db, key) != 0) {
> +               abort();
> +       }
> +
> +       if (tdb_delete(db, key) != 0) {
> +               abort();
> +       }
> +       if (tdb_chainunlock(db, key) != 0) {
> +               abort();
> +       }
> +}
> 
> seems to work fine. So I don't think there's
> a problem with deleting a record under a chainlock...
> 
> (I'm pretty sure those locks are recursive).

I tried adding a record with tdbtool insert, killing the corresponding
smbd process and watch the record disappear. So far i only get -1 from
tdb_delete. Maybe something is wrong with this test, i am still digging
through it.

Christof