[PATCH] Cleanup - remove unused functionality.

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

[PATCH] Cleanup - remove unused functionality.

Samba - samba-technical mailing list
More removal of talloc_autofree_context()
from completely unused code.

I'm planning to remove this entirely from
our codebase so we can make a start on adding
threaded code into Samba.

Please review and push if happy !

Jeremy.

0001-lib-param-Remove-lpcfg_register_defaults_hook.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> More removal of talloc_autofree_context()
> from completely unused code.
>
> I'm planning to remove this entirely from
> our codebase so we can make a start on adding
> threaded code into Samba.
>
> Please review and push if happy !
I'm slightly worried about removal of the lpcfg_register_defaults_hook()
and friends. Sure, Openchange seems to be dead but people still use
Zentyal's fork which was updated a year ago.

I hope anyone who still cares about Openchange/Zentyal version would
speak up here. If not, then be it.

--
/ Alexander Bokovoy

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy wrote:

> On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> > More removal of talloc_autofree_context()
> > from completely unused code.
> >
> > I'm planning to remove this entirely from
> > our codebase so we can make a start on adding
> > threaded code into Samba.
> >
> > Please review and push if happy !
> I'm slightly worried about removal of the lpcfg_register_defaults_hook()
> and friends. Sure, Openchange seems to be dead but people still use
> Zentyal's fork which was updated a year ago.

It's a horrible external abuse of our code, which
prevents us from modernizing our internals.

I want rid of these monkeys on our back.

If we want to allow this - we need to decide
on a *documented* *supported* mechanism of
allowing this, that is thought through correctly
and doesn't pollute our code with global state.

lpcfg_register_defaults_hook() is *not* that
mechanism.

> I hope anyone who still cares about Openchange/Zentyal version would
> speak up here. If not, then be it.

Openchange is dead. If there are required
uses for this, then external projects need
to request an interface for it that we can
design togther and support going forward.

In the meantime, all our uses of talloc_autofree_context()
must die.

Please RB+ and push :-).

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy wrote:

> On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> > More removal of talloc_autofree_context()
> > from completely unused code.
> >
> > I'm planning to remove this entirely from
> > our codebase so we can make a start on adding
> > threaded code into Samba.
> >
> > Please review and push if happy !
> I'm slightly worried about removal of the lpcfg_register_defaults_hook()
> and friends. Sure, Openchange seems to be dead but people still use
> Zentyal's fork which was updated a year ago.
>
> I hope anyone who still cares about Openchange/Zentyal version would
> speak up here. If not, then be it.

Just to be blunt. Zentyal is a commercial company that sells
a distro that uses our code as part of it.

There's nothing wrong with that, all of our vendors do
much the same thing and may of them employ us (which
I'm sure you'll agree is a good thing :-).

However, Zentyal is the only company that currently has moved
the technical debt of maintaining changes for their product
into our code, not a private fork that they must maintain.

Isilon did the same thing with their changes to
our VFS to support their OneFS filesystem, and
in retrospect that was a mistake.

After Isilon moved off the Samba codebase we were
able to remove their custom code from Samba.

We need to do the same thing here. If this change
is essential for Zentyal then it has to be a private
patch that they carry, not something that prevents
modernizing our code.

Hope that explains why I want you to RB+ this
patch and push it :-).

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On ti, 18 huhti 2017, Jeremy Allison wrote:

> On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy wrote:
> > On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> > > More removal of talloc_autofree_context()
> > > from completely unused code.
> > >
> > > I'm planning to remove this entirely from
> > > our codebase so we can make a start on adding
> > > threaded code into Samba.
> > >
> > > Please review and push if happy !
> > I'm slightly worried about removal of the lpcfg_register_defaults_hook()
> > and friends. Sure, Openchange seems to be dead but people still use
> > Zentyal's fork which was updated a year ago.
>
> It's a horrible external abuse of our code, which
> prevents us from modernizing our internals.
>
> I want rid of these monkeys on our back.
All I want to see is a louder announcement that shatter is coming.
Without being sound how could they know it is a scheduled drop?
Announcing the removal under "unused functionality" cleanup is living us
without any towel. Have you put these plans somewhere else half a year
ago, perhaps?

> If we want to allow this - we need to decide
> on a *documented* *supported* mechanism of
> allowing this, that is thought through correctly
> and doesn't pollute our code with global state.
>
> lpcfg_register_defaults_hook() is *not* that
> mechanism.
+1.


>
> > I hope anyone who still cares about Openchange/Zentyal version would
> > speak up here. If not, then be it.
>
> Openchange is dead. If there are required
> uses for this, then external projects need
> to request an interface for it that we can
> design togther and support going forward.
>
> In the meantime, all our uses of talloc_autofree_context()
> must die.
We have talloc_autofree_context() used by SSSD as well. Are you planning
to kill talloc_autofree_context() too? Not that SSSD needs it
desperately but it is used there.


--
/ Alexander Bokovoy

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 09:07:26PM +0300, Alexander Bokovoy wrote:

> On ti, 18 huhti 2017, Jeremy Allison wrote:
> > On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy wrote:
> > > On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> > > > More removal of talloc_autofree_context()
> > > > from completely unused code.
> > > >
> > > > I'm planning to remove this entirely from
> > > > our codebase so we can make a start on adding
> > > > threaded code into Samba.
> > > >
> > > > Please review and push if happy !
> > > I'm slightly worried about removal of the lpcfg_register_defaults_hook()
> > > and friends. Sure, Openchange seems to be dead but people still use
> > > Zentyal's fork which was updated a year ago.
> >
> > It's a horrible external abuse of our code, which
> > prevents us from modernizing our internals.
> >
> > I want rid of these monkeys on our back.
> All I want to see is a louder announcement that shatter is coming.
> Without being sound how could they know it is a scheduled drop?
> Announcing the removal under "unused functionality" cleanup is living us
> without any towel. Have you put these plans somewhere else half a year
> ago, perhaps?

This is for 4.7+. It's not like I'm removing from
current releases.

I can add a note to the WHATSNEW if you like ?

The problem is this is a one-project-private
hack that somehow has become embedded in Samba
code and is now a barrier to fixing real bugs
and modernizing our code.

If it's essential to Zentyal's fork they
need to carry it as a private patch, that's
all I'm saying. That's what a fork means
after all. This interface isn't promised
as a public ABI.

Everyone else does the same for custom changes they
need.

> > and doesn't pollute our code with global state.
> >
> > lpcfg_register_defaults_hook() is *not* that
> > mechanism.
> +1.

Well I'm glad we agree on that :-).

> We have talloc_autofree_context() used by SSSD as well. Are you planning
> to kill talloc_autofree_context() too? Not that SSSD needs it
> desperately but it is used there.

Not at all ! talloc_autofree_context() is part of
our external talloc ABI and will never go away.

I just don't want it used inside Samba code,
that's all.

The reason is that it installs a global atexit()
handler and when used with talloc_enable_null_tracking()
(which Samba code currently does, I want rid
of that too :-) it makes it *impossible* for
talloc code to be used safely by threads.

We have to fix this for Samba to move us
forward.

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
On ti, 18 huhti 2017, Jeremy Allison wrote:

> On Tue, Apr 18, 2017 at 09:07:26PM +0300, Alexander Bokovoy wrote:
> > On ti, 18 huhti 2017, Jeremy Allison wrote:
> > > On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy wrote:
> > > > On ti, 18 huhti 2017, Jeremy Allison via samba-technical wrote:
> > > > > More removal of talloc_autofree_context()
> > > > > from completely unused code.
> > > > >
> > > > > I'm planning to remove this entirely from
> > > > > our codebase so we can make a start on adding
> > > > > threaded code into Samba.
> > > > >
> > > > > Please review and push if happy !
> > > > I'm slightly worried about removal of the lpcfg_register_defaults_hook()
> > > > and friends. Sure, Openchange seems to be dead but people still use
> > > > Zentyal's fork which was updated a year ago.
> > >
> > > It's a horrible external abuse of our code, which
> > > prevents us from modernizing our internals.
> > >
> > > I want rid of these monkeys on our back.
> > All I want to see is a louder announcement that shatter is coming.
> > Without being sound how could they know it is a scheduled drop?
> > Announcing the removal under "unused functionality" cleanup is living us
> > without any towel. Have you put these plans somewhere else half a year
> > ago, perhaps?
>
> This is for 4.7+. It's not like I'm removing from
> current releases.
>
> I can add a note to the WHATSNEW if you like ?
Yes, please add the note and then my RB+.
 

> The problem is this is a one-project-private
> hack that somehow has become embedded in Samba
> code and is now a barrier to fixing real bugs
> and modernizing our code.
>
> If it's essential to Zentyal's fork they
> need to carry it as a private patch, that's
> all I'm saying. That's what a fork means
> after all. This interface isn't promised
> as a public ABI.
>
> Everyone else does the same for custom changes they
> need.
I do agree with this approach. As I said, I wanted this to be visible on
the list rather than burried in the code. Process-related policies not
necessarily good when only enforced through the code.
 

> > We have talloc_autofree_context() used by SSSD as well. Are you planning
> > to kill talloc_autofree_context() too? Not that SSSD needs it
> > desperately but it is used there.
>
> Not at all ! talloc_autofree_context() is part of
> our external talloc ABI and will never go away.
>
> I just don't want it used inside Samba code,
> that's all.
>
> The reason is that it installs a global atexit()
> handler and when used with talloc_enable_null_tracking()
> (which Samba code currently does, I want rid
> of that too :-) it makes it *impossible* for
> talloc code to be used safely by threads.
>
> We have to fix this for Samba to move us
> forward.
Yep. Please note that some issues cannot be solved without glibc be
fixed. MIT Kerberos, for example, loads plugins and makes sure they are
unloaded when not used anymore. This freaks out glibc code due to
undefined behavior in ELF spec. It was fixed recently (December 2016) in
Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1398370

--
/ Alexander Bokovoy

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

Re: [PATCH] Cleanup - remove unused functionality. -- OpenChange break

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-04-18 at 11:18 -0700, Jeremy Allison via samba-technical
wrote:

> On Tue, Apr 18, 2017 at 09:07:26PM +0300, Alexander Bokovoy wrote:
> > On ti, 18 huhti 2017, Jeremy Allison wrote:
> > > On Tue, Apr 18, 2017 at 08:46:48PM +0300, Alexander Bokovoy
> > > wrote:
> > > >
> > > > I'm slightly worried about removal of the
> > > > lpcfg_register_defaults_hook()
> > > > and friends. Sure, Openchange seems to be dead but people still
> > > > use
> > > > Zentyal's fork which was updated a year ago.
> > >
> > > It's a horrible external abuse of our code, which
> > > prevents us from modernizing our internals.
> > >
> > > I want rid of these monkeys on our back.
> >
> > All I want to see is a louder announcement that shatter is coming.
> > Without being sound how could they know it is a scheduled drop?
> > Announcing the removal under "unused functionality" cleanup is
> > living us
> > without any towel. Have you put these plans somewhere else half a
> > year
> > ago, perhaps?
>
> This is for 4.7+. It's not like I'm removing from
> current releases.
>
> I can add a note to the WHATSNEW if you like ?

That would be a good start.

> The problem is this is a one-project-private
> hack that somehow has become embedded in Samba
> code and is now a barrier to fixing real bugs
> and modernizing our code.

As hacks go, it is on par with the hacks that VFS modules, even in-
tree, do when they override smb.conf settings on load.  It is just that
in the source4 code we don't have a global loadparm context to refer
to.

> If it's essential to Zentyal's fork they
> need to carry it as a private patch, that's
> all I'm saying. That's what a fork means
> after all. This interface isn't promised
> as a public ABI.

I don't see it as essential - nothing this particular API provides
can't be provided by an installation script (like Samba's provision)
that writes out smb.conf values.  It was added quite recently (2015) in
an attempt to make Openchange easier to install, and so more viable as
a project:

commit 782e8d6aabd6535448992f1ee69f25c71e8f2b6c
Author: Jelmer Vernooij <[hidden email]>
Date:   Mon Mar 16 12:30:27 2015 +1300

    lib/param: Add hook that allows modification of default settings.
    
    This is useful for reducing the amount of configuration necessary for
    OpenChange.
    
    The hook is ideally registered from a plugin initialization function,
    so that it automatically gets used whenever the plugin is installed.
    
    This makes it possible for plugins to e.g. extend the default value for
    the list of enabled dcerpc endpoint services.
    
    Like all our interfaces, callers are expected to use this API
    responsibly. For example, OpenChange should only enable its DCE/RPC
    interface if it has been provisioned.
    
    Change-Id: Ic8bacdd8b4c92a2a4b97cfa1a50dc41365b78071
    Signed-Off-By: Jelmer Vernooij <[hidden email]>
    
    Reviewed-by: Andrew Bartlett <[hidden email]>


It wasn't added for Zentyal, they are just the last firm to have left
the OpenChange effort.  They no longer advertise it as part of their
features, and their fork has been dormant for a year also (April 4 last
year as of this writing):
https://github.com/zentyal/openchange/commits/master

Others continue to try and keep it working, eg:
https://github.com/SlavekB/openchange/commits/master

https://github.com/SlavekB/openchange/commit/a4d336cc040db88a0a2e41bc8b
ee43d207e24ad1

> Everyone else does the same for custom changes they
> need.

These 'horrible hacks' (there are worse interfaces exposed, like the
DCE/RPC server internals, which we keep breaking very release or so)
are the alternative to OpenChange having a full fork of Samba, or like
CTDB needing to be merged into our tree.

All that said, do be careful what you wish for.  In Debian, I had folks
politely ask if Samba was willing to maintain OpenChange:  I declined.
:-)

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Loading...