[PATCHES] Cleanup dead records in messages.tdb

[Christof Schmitt]

On Thu, Apr 03, 2014 at 03:05:02PM -0700, Christof Schmitt wrote:
> 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.

The patch works as expected, i just forgot to add the 0 byte terminator
in the key of my fake tdb record. Sorry for the noise.

Christof