[PATCHES] Cleanup dead records in messages.tdb

[Jeremy Allison]

On Thu, Apr 03, 2014 at 02:26:29PM -0700, Christof Schmitt wrote:
> On Wed, Apr 02, 2014 at 09:38:58PM -0700, Christof Schmitt wrote:
> > On Wed, Apr 02, 2014 at 06:06:47PM -0700, Jeremy Allison wrote:
> > > On Wed, Apr 02, 2014 at 05:21:51PM -0700, Christof Schmitt wrote:
> > > > minor question would be why you have messaging_tdb_cleanup return a
> > > > status, only to ignore it in the caller.
> > > 
> > > We ignore it *now*, as there's not much
> > > we can really do with it (maybe DEBUG
> > > message ?). But it is good pactice to
> > > return NTSTATUS error codes whenever
> > > possible, so I always try and do so.
> > 
> > I would have made the function return nothing, and only add the returned
> > status when it is actually needed. Anyway, this is just a minor point.
> > 
> > > 
> > > > Overall it looks good:
> > > > Reviewed-by: Christof Schmitt <cs at samba.org>
> > > 
> > > Thanks ! I'll push tomorrow ! Do you want to
> > > log a bug so we can get it back-ported to
> > > 4.0.next and 4.1.next ?
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=10534
> 
> 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...

Jeremy.