[PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

Samba - samba-technical mailing list
Whilst fixing the previous bug in reinitializing
s4 imessaging after fork(), I thought there should
be a way to move the /bin/samba event context off
the talloc_autofree context, which was causing all
the trouble.

Here is a patchset that does some cleanups, and
moves the top level event context into a state
struct that is allocated and freed up correctly
without having to use the autofree context.

Again, this won't affect Andrews's ldap multi-process
patch and can go in before or after.

Description follows:

------------------------------------------------------
The two below are merely code cleanups:

#1: Add missing error return checks in imessaging_init().
#2: Whitespace and 80+ column cleanup in source4/smbd/server.c
------------------------------------------------------

The meat of the patch follows below, split into
micro-changes for easy digestion :-):

#3: Create a server_state struct. Use it to hold a pointer
to the top level event context, but leave that allocated
on autofree for now.

------------------------------------------------------

Minor changes:

#4: Plumb server_state into server_stdin_handler()
#5: Plumb server_state into max_runtime_handler()
#6: Plumb server_state into setup_parent_messaging()
#7: Add error return check for tevent_add_fd() tevent_add_timer()
#8: Add a tevent SIGTERM handler - simplify by removing global state.

------------------------------------------------------

Core changes:

#9: Change the event context destructor to unref
only msg_dgm_ref entries that are pointing to it.

#10: Make state the parent of the top level event
context, not the autofree context. Ensure that
TALLOC_FREE(state) is called on all exit paths.

#11: Make state the parent of open_schannel_session_store().
Don't use the autofree context anymore.
------------------------------------------------------

Passes full autobuild.

Please review and push if happy !

Cheers,

        Jeremy.

s4_server_cleanup5 (37K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

Samba - samba-technical mailing list
On Mon, Apr 10, 2017 at 08:11:44PM -0700, Jeremy Allison via samba-technical wrote:
> Whilst fixing the previous bug in reinitializing
> s4 imessaging after fork(), I thought there should
> be a way to move the /bin/samba event context off
> the talloc_autofree context, which was causing all
> the trouble.

Cool stuff :-)

Attached find a patchset with a few to-squash patches and a question.
No formal review yet, but some minor cleanups (IMHO at least).

Volker

patch.txt (42K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

Samba - samba-technical mailing list
On Tue, Apr 11, 2017 at 08:02:15AM +0200, Volker Lendecke wrote:

> On Mon, Apr 10, 2017 at 08:11:44PM -0700, Jeremy Allison via samba-technical wrote:
> > Whilst fixing the previous bug in reinitializing
> > s4 imessaging after fork(), I thought there should
> > be a way to move the /bin/samba event context off
> > the talloc_autofree context, which was causing all
> > the trouble.
>
> Cool stuff :-)
>
> Attached find a patchset with a few to-squash patches and a question.
> No formal review yet, but some minor cleanups (IMHO at least).

I'll post a new patchset + the squashed patches soon.

Here is the answer to your question:

---------------------------------------------------------------------
Subject: [PATCH 13/15] VL: I'd love to see why this is necessary. What fails
 without it?

I can see it's cleaner and the next commit (probably) builds upon it,
but the actual use case would be great to understand

s4: messaging: When talloc_free()'ing an event context, only remove msg_dgm_ref's that point to *that* context.
---------------------------------------------------------------------

Nothing fails without it and it's not strictly necessary.

I did it as a defensive programming change to make sure we
only talloc_free msg_dgm_ref from a event_context destructor
that is pointing to itself.

What made me think about this was I started thinking
about the nested event loop problem, and how we might
eventually remove all of these in the s4 code and turn
them into correctly embedded contexts as we do in smbd.

(Vague plan along those lines was at some point
to remove the call to tevent_loop_allow_nesting() and see what
crashes I get when running 'make test' :-).

If we did that, we might end up with multiple calls
to imessaging_init() from one process down the road.
The data structures inside source4/lib/messaging.c
contain a linked list of struct imessaging_context
pointers, and each of those has a possibly different
struct tevent_context pointer.

As I was messing with this areas it seemed like a
harmless thing to fix (and I knew if I didn't do
it I might forget later :-).

So defensively it might help in the future, but it's not
needed right now and doesn't prevent any crash. If you really
hate it and want to leave the current call to
imessaging_dgm_unref_all() alone I'm OK with that too.

Cheers,

        Jeremy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Apr 11, 2017 at 08:02:15AM +0200, Volker Lendecke wrote:

> On Mon, Apr 10, 2017 at 08:11:44PM -0700, Jeremy Allison via samba-technical wrote:
> > Whilst fixing the previous bug in reinitializing
> > s4 imessaging after fork(), I thought there should
> > be a way to move the /bin/samba event context off
> > the talloc_autofree context, which was causing all
> > the trouble.
>
> Cool stuff :-)
>
> Attached find a patchset with a few to-squash patches and a question.
> No formal review yet, but some minor cleanups (IMHO at least).
Updated patchset with your cleanups incorporated.
Extra explaination on the defensive programming
patch.

Let me know if this works !

Cheers,

        Jeremy.

s4_server_cleanup6 (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Remove use of talloc_autofree_context() from source4/smbd/server.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Apr 11, 2017 at 09:29:10AM -0700, Jeremy Allison wrote:
> So defensively it might help in the future, but it's not
> needed right now and doesn't prevent any crash. If you really
> hate it and want to leave the current call to
> imessaging_dgm_unref_all() alone I'm OK with that too.

No, I actually like it. I just did not understand the reasoning behind
it. Reviewed by me for the newly sent patchset, but I'd like metze to
also take a brief look.

Thanks!

Volker

Loading...