[PATCH] Change module interface to pass in a TALLOC_CTX pointer.

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

[PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
Currently the initialization function of all our modules are called with a
function signature of:

NTSTATUS XXX_init(void)

The following patch changes this to be:

NTSTATUS XXX_init(TALLOC_CTX *ctx)

instead. The patch (for 4.7.x naturally) merely changes
the definition of the initialization functions and their
callers, and implements *no* logic changes (all of the changed
callers in this patch pass NULL in as the new TALLOC_CTX *ctx
and the modules ignore it).

As it's a change to the module interfaces, I have also
incremented the version numbers for all loadable modules,
and added text to the WHATSNEW and to the module documentation.

The reason I want this change is that currently many modules
do something like:

NTSTATUS XXX_init(void)
{
        call_register_function(...stuff..)
....
}

and the called function looks like:

void call_register_function(...params...)
{
        backend = talloc(talloc_autofree_context(), .... );
....
}

Essentially the modules don't get access to a long-lived
talloc context from their caller so end up using the global
talloc_autofree_context() so they look cleaner when run under
valgrind (i.e. at least someone frees this stuff on exit).

This is a nasty, lazy way of memory management and I really
want the calling program to hand them down a long-lived
talloc context (usually initialized in the main() function
of the program) and manage doing the talloc_free() just
before doing the exit itself.

I'd appreciate people looking this over to ensure I've
changed all the relevent version numbers, ABI numbers etc.

It passes a full autobuild make test. Once it gets in I
can start fixing the underlying memory management by
passing in a real talloc context that the modules can
use for their long-term initialization use instead of
talloc_autofree_context().

Let me know what you think. It looks a bigger change
than it is, most of it is pure boilerplate changes.

Cheers,

        Jeremy.

0001-lib-modules-Change-XXX_init-interface-from-XXX_init-.patch (112K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
Hi Jeremy,

On Fri, Apr 21, 2017 at 03:01:41AM +0000, Jeremy Allison wrote:

> Currently the initialization function of all our modules are called with a
> function signature of:
>
> NTSTATUS XXX_init(void)
>
> The following patch changes this to be:
>
> NTSTATUS XXX_init(TALLOC_CTX *ctx)
>
> instead. The patch (for 4.7.x naturally) merely changes
> the definition of the initialization functions and their
> callers, and implements *no* logic changes (all of the changed
> callers in this patch pass NULL in as the new TALLOC_CTX *ctx
> and the modules ignore it).
Nice!

One thing: you missed the static initializers and declarations, eg for the VFS:

vfs_init_custom() uses "static_init_vfs;" which expands to

  #define static_init_vfs { vfs_default_init();  vfs_posixacl_init();  vfs_dfs_samba4_init(); }

But the vfs init functions now take a TALLOC_CTX arg, eg

   NTSTATUS vfs_default_init(TALLOC_CTX *ctx)

The compiler doesn't complain, because the static_decl_vfs contains the same
wrong function signatures:

  #define static_decl_vfs extern NTSTATUS vfs_default_init(void); extern NTSTATUS vfs_posixacl_init(void); extern NTSTATUS vfs_dfs_samba4_init(void);

Attached patch should fix this. Fixup your commit with mine if happy.

-slow

static-init.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 02:29:23PM +0200, Ralph Böhme wrote:

> Hi Jeremy,
>
> On Fri, Apr 21, 2017 at 03:01:41AM +0000, Jeremy Allison wrote:
> > Currently the initialization function of all our modules are called with a
> > function signature of:
> >
> > NTSTATUS XXX_init(void)
> >
> > The following patch changes this to be:
> >
> > NTSTATUS XXX_init(TALLOC_CTX *ctx)
> >
> > instead. The patch (for 4.7.x naturally) merely changes
> > the definition of the initialization functions and their
> > callers, and implements *no* logic changes (all of the changed
> > callers in this patch pass NULL in as the new TALLOC_CTX *ctx
> > and the modules ignore it).
>
> Nice!
>
> One thing: you missed the static initializers and declarations, eg for the VFS:
>
> vfs_init_custom() uses "static_init_vfs;" which expands to
>
>   #define static_init_vfs { vfs_default_init();  vfs_posixacl_init();  vfs_dfs_samba4_init(); }
>
> But the vfs init functions now take a TALLOC_CTX arg, eg
>
>    NTSTATUS vfs_default_init(TALLOC_CTX *ctx)
>
> The compiler doesn't complain, because the static_decl_vfs contains the same
> wrong function signatures:
>
>   #define static_decl_vfs extern NTSTATUS vfs_default_init(void); extern NTSTATUS vfs_posixacl_init(void); extern NTSTATUS vfs_dfs_samba4_init(void);
>
> Attached patch should fix this. Fixup your commit with mine if happy.
Thanks for that Ralph ! Much appreciated.

Minor waf complaint....

If you do:

git grep static_decl_vfs

You can't find the definition of that *anywhere*,
because it's a magic waf construct made out of
unicorns, rainbows and python. Same for:

git grep static_init_nss

Which explains why I didn't find them :-(.

I *hate* magic declarations/definitions, or
anything you can't 'git grep' for and find.

How did you notice the error ? Are there
any other places I need to know about
where declarations/definitions are constructed ?

Once this goes in I should be able to safely
remove all talloc_autofree_context() calls out
of everywhere except ldb, which has it's own
custom module init interface that takes a
'const char *version' parameter. I plan to
look at that later.

Here is your patch squashed with mine and your
Signed-off-by: added.

Please push if happy !

Cheers,

        Jeremy.

0001-lib-modules-Change-XXX_init-interface-from-XXX_init-.patch (129K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 10:03:00AM -0700, Jeremy Allison wrote:

> On Fri, Apr 21, 2017 at 02:29:23PM +0200, Ralph Böhme wrote:
> > Hi Jeremy,
> >
> > On Fri, Apr 21, 2017 at 03:01:41AM +0000, Jeremy Allison wrote:
> > > Currently the initialization function of all our modules are called with a
> > > function signature of:
> > >
> > > NTSTATUS XXX_init(void)
> > >
> > > The following patch changes this to be:
> > >
> > > NTSTATUS XXX_init(TALLOC_CTX *ctx)
> > >
> > > instead. The patch (for 4.7.x naturally) merely changes
> > > the definition of the initialization functions and their
> > > callers, and implements *no* logic changes (all of the changed
> > > callers in this patch pass NULL in as the new TALLOC_CTX *ctx
> > > and the modules ignore it).
> >
> > Nice!
> >
> > One thing: you missed the static initializers and declarations, eg for the VFS:
> >
> > vfs_init_custom() uses "static_init_vfs;" which expands to
> >
> >   #define static_init_vfs { vfs_default_init();  vfs_posixacl_init();  vfs_dfs_samba4_init(); }
> >
> > But the vfs init functions now take a TALLOC_CTX arg, eg
> >
> >    NTSTATUS vfs_default_init(TALLOC_CTX *ctx)
> >
> > The compiler doesn't complain, because the static_decl_vfs contains the same
> > wrong function signatures:
> >
> >   #define static_decl_vfs extern NTSTATUS vfs_default_init(void); extern NTSTATUS vfs_posixacl_init(void); extern NTSTATUS vfs_dfs_samba4_init(void);
> >
> > Attached patch should fix this. Fixup your commit with mine if happy.
>
> Thanks for that Ralph ! Much appreciated.
>
> Minor waf complaint....
>
> If you do:
>
> git grep static_decl_vfs
>
> You can't find the definition of that *anywhere*,
> because it's a magic waf construct made out of
> unicorns, rainbows and python. Same for:

well, I'm by no means a waf fan, but this can happen with other buildsystems as
well. :)

>
> git grep static_init_nss
>
> Which explains why I didn't find them :-(.

hehe

> I *hate* magic declarations/definitions, or
> anything you can't 'git grep' for and find.

I use cscope, that finds it. ctags should look inside bin as well.

> How did you notice the error ?

Because I knew it was there. :)

> Are there any other places I need to know about
> where declarations/definitions are constructed ?

No. The only place (I know of :)))) ) is source3/wscript and I already fixed it
for you.

> Once this goes in I should be able to safely
> remove all talloc_autofree_context() calls out
> of everywhere except ldb, which has it's own
> custom module init interface that takes a
> 'const char *version' parameter. I plan to
> look at that later.
>
> Here is your patch squashed with mine and your
> Signed-off-by: added.
>
> Please push if happy !

will do.

-slow

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

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 07:08:15PM +0200, Ralph Böhme wrote:
> > Please push if happy !
>
> will do.

done.

-slow

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

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Friday, 21 April 2017 19:03:00 CEST Jeremy Allison wrote:

> If you do:
>
> git grep static_decl_vfs
>
> You can't find the definition of that *anywhere*,
> because it's a magic waf construct made out of
> unicorns, rainbows and python. Same for:
>
> git grep static_init_nss
>
> Which explains why I didn't find them :-(.

I complained about this stuff already long ago. We should either have
everything as module (plugin) or compiled into the source. And not some as
module and some static and if you compile the wrong one as a module everything
stops working.

Just my 2 cents.


        Andreas

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

Re: [PATCH] Change module interface to pass in a TALLOC_CTX pointer.

Samba - samba-technical mailing list
On Fri, Apr 21, 2017 at 09:45:29PM +0200, Andreas Schneider via samba-technical wrote:

> On Friday, 21 April 2017 19:03:00 CEST Jeremy Allison wrote:
> > If you do:
> >
> > git grep static_decl_vfs
> >
> > You can't find the definition of that *anywhere*,
> > because it's a magic waf construct made out of
> > unicorns, rainbows and python. Same for:
> >
> > git grep static_init_nss
> >
> > Which explains why I didn't find them :-(.
>
> I complained about this stuff already long ago. We should either have
> everything as module (plugin) or compiled into the source. And not some as
> module and some static and if you compile the wrong one as a module everything
> stops working.
>
> Just my 2 cents.

Yeah, but I don't want to boil the ocean today. Just
exterminating a particularly venemous pest
(talloc_autofree_context() :-).

Loading...