VFS connection data cleanup with multiple modules

[Jeremy Allison]

On Wed, Oct 01, 2008 at 03:46:34PM -0400, Constantine Vetoshev wrote:
> On Tue, Sep 30, 2008 at 4:54 PM, Jeremy Allison <jra at samba.org> wrote:
> >> When I disconnect from the share, I expect that release_data_1 and
> >> release_data_2 both run, but logging shows that only release_data_1
> >> runs. I can force release_data_2 to run by putting an explicit call to
> >> it from a disconnect callback for vfs_object_2, but that doesn't seem
> >> right.
> >>
> >> Am I missing something about how the release functions should work?
> >
> > It should get called from conn_free_internal() when the
> > connection struct is removed. See this code :
> >
> >        /* Free vfs_connection_struct */
> >        handle = conn->vfs_handles;
> >        while(handle) {
> >                DLIST_REMOVE(conn->vfs_handles, handle);
> >                thandle = handle->next;
> >                if (handle->free_data)
> >                        handle->free_data(&handle->data);
> >                handle = thandle;
> >        }
> >
> > which should call all the handle release functions in
> > reverse order of registration.
> 
> After poking at the code with gdb, I think I see a minor bug here.
> Before the loop enters, conn->vfs_handles looks good, and
> conn->vfs_handles->next also looks correct (as do all subsequent
> ->next pointers). Then, in the first iteration, after DLIST_REMOVE,
> handle->next is NULL, so thandle is set to NULL. Looking at the
> implementation of DLIST_REMOVE, I understand why: the p argument sets
> its next and prev pointers to NULL if list != p, which it definitely
> is not after list gets assigned to p->next. The code then sets handle
> to thandle (which is NULL), and the loop terminates after checking
> handle. In effect, free_data gets called on just the first handle.
> 
> A trivial workaround is to just set thandle before calling DLIST_REMOVE.
> 
> This code hasn't changed from 3.0.28a and 3.2.4, so the problem should
> affect 3.2.4.

Great catch - thanks for the analysis - I'm fixing in all branches
now !

Jeremy.