[PATCH] Run 'samba' daemon in foreground

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
Hi,

attached is a patch to address an issue with systemd and Samba daemons running
in notify mode. In this mode we should not double fork. So we should start the
daemons with --foreground. Also systemd will take care of the process group we
should not handle that in Samba.

For this I've added --foreground to the 'samba' daemon.


Review is much appreciated. If OK please push.


Thanks,


        Andreas


--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

samba-forground.patch1.txt (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
On Mon, 27 Nov 2017 19:43:01 +0100
Andreas Schneider via samba-technical <[hidden email]>
wrote:

> Hi,
>
> attached is a patch to address an issue with systemd and Samba
> daemons running in notify mode. In this mode we should not double
> fork. So we should start the daemons with --foreground. Also systemd
> will take care of the process group we should not handle that in
> Samba.
>
> For this I've added --foreground to the 'samba' daemon.
>
>
> Review is much appreciated. If OK please push.
>
>
> Thanks,
>
>
> Andreas
>
>

NACK, until you explain if this affects distros that don't use
systemd ?

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
technical wrote:

> Hi,
>
> attached is a patch to address an issue with systemd and Samba daemons running
> in notify mode. In this mode we should not double fork. So we should start the
> daemons with --foreground. Also systemd will take care of the process group we
> should not handle that in Samba.
>
> For this I've added --foreground to the 'samba' daemon.
>
>
> Review is much appreciated. If OK please push.
>

My main question comes from not really understanding what a session ID
is in unix.  Your patch makes --no-process-group also change us to call
getsid(), which was unconditionally false previously.

Is that reasonable?  If so, can you just write up what exactly it means
in the commit message?

BTW, like like in smbd, it will listen for a fifo/socket on STDIN.
This won't impact on systemd as by default it uses /dev/null and that
is ignored.

The options here are a confusing mess, so noting the differences with
-i (--interactive) would be helpful.  As I read it, the only difference
is that with -i logs got to STDOUT and with --foreground
become_daemon() is called, potentially allowing --no-process-group to
trigger the setsid()?  

I can assure Rowland that adding the extra option won't impact on
systems not using systemd.

Thanks,

Andrew Bartlett

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





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
On Tue, 28 Nov 2017 10:33:22 +1300
Andrew Bartlett via samba-technical <[hidden email]>
wrote:

> On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
> technical wrote:
> > Hi,
> >
> > attached is a patch to address an issue with systemd and Samba
> > daemons running in notify mode. In this mode we should not double
> > fork. So we should start the daemons with --foreground. Also
> > systemd will take care of the process group we should not handle
> > that in Samba.
> >
> > For this I've added --foreground to the 'samba' daemon.
> >
> >
> > Review is much appreciated. If OK please push.
> >
>
> My main question comes from not really understanding what a session ID
> is in unix.  Your patch makes --no-process-group also change us to
> call getsid(), which was unconditionally false previously.
>
> Is that reasonable?  If so, can you just write up what exactly it
> means in the commit message?
>
> BTW, like like in smbd, it will listen for a fifo/socket on STDIN.
> This won't impact on systemd as by default it uses /dev/null and that
> is ignored.
>
> The options here are a confusing mess, so noting the differences with
> -i (--interactive) would be helpful.  As I read it, the only
> difference is that with -i logs got to STDOUT and with --foreground
> become_daemon() is called, potentially allowing --no-process-group to
> trigger the setsid()?  
>
> I can assure Rowland that adding the extra option won't impact on
> systems not using systemd.
>
> Thanks,
>
> Andrew Bartlett
>

Thanks Andrew, I will withdraw my NAK on the basis this is an option
i.e. it isn't forced on anybody and the 'old' way will still work

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Monday, 27 November 2017 20:01:41 CET Rowland Penny via samba-technical
wrote:

> On Mon, 27 Nov 2017 19:43:01 +0100
> Andreas Schneider via samba-technical <[hidden email]>
>
> wrote:
> > Hi,
> >
> > attached is a patch to address an issue with systemd and Samba
> > daemons running in notify mode. In this mode we should not double
> > fork. So we should start the daemons with --foreground. Also systemd
> > will take care of the process group we should not handle that in
> > Samba.
> >
> > For this I've added --foreground to the 'samba' daemon.
> >
> >
> > Review is much appreciated. If OK please push.
> >
> >
> > Thanks,
> >
> > Andreas
>
> NACK, until you explain if this affects distros that don't use
> systemd ?

I don't think that those distros use pass the newly added option --foreground
option by default to the samba binary ...


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Monday, 27 November 2017 22:33:22 CET Andrew Bartlett via samba-technical
wrote:

> On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
>
> technical wrote:
> > Hi,
> >
> > attached is a patch to address an issue with systemd and Samba daemons
> > running in notify mode. In this mode we should not double fork. So we
> > should start the daemons with --foreground. Also systemd will take care
> > of the process group we should not handle that in Samba.
> >
> > For this I've added --foreground to the 'samba' daemon.
> >
> >
> > Review is much appreciated. If OK please push.
>
> My main question comes from not really understanding what a session ID
> is in unix.  Your patch makes --no-process-group also change us to call
> getsid(), which was unconditionally false previously.
>
> Is that reasonable?  If so, can you just write up what exactly it means
> in the commit message?
If you have a SysV Daemon, you should call setsid() to detach from any
terminal and create an independent session.

If we are running in systemd we should run in forground mode and not call
setsid()!

See

https://www.freedesktop.org/software/systemd/man/daemon.html

how SysV Daemons should be implemented.

> The options here are a confusing mess, so noting the differences with
> -i (--interactive) would be helpful.  As I read it, the only difference
> is that with -i logs got to STDOUT and with --foreground
> become_daemon() is called, potentially allowing --no-process-group to
> trigger the setsid()?

No, --no-process-group does NOT trigger setsid()!

samba --no-process-group:
no_process_group=true  =>  become_daemon(no_session=true)

if (!no_session) {
  setsid()
}

Updated patchset attached.


        Andreas


--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

samba-forground.patch2.txt (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
On Tue, 28 Nov 2017 10:59:15 +0100
Andreas Schneider via samba-technical <[hidden email]>
wrote:

> On Monday, 27 November 2017 22:33:22 CET Andrew Bartlett via
> samba-technical wrote:
> > On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
> >
> > technical wrote:
> > > Hi,
> > >
> > > attached is a patch to address an issue with systemd and Samba
> > > daemons running in notify mode. In this mode we should not double
> > > fork. So we should start the daemons with --foreground. Also
> > > systemd will take care of the process group we should not handle
> > > that in Samba.
> > >
> > > For this I've added --foreground to the 'samba' daemon.
> > >
> > >
> > > Review is much appreciated. If OK please push.
> >
> > My main question comes from not really understanding what a session
> > ID is in unix.  Your patch makes --no-process-group also change us
> > to call getsid(), which was unconditionally false previously.
> >
> > Is that reasonable?  If so, can you just write up what exactly it
> > means in the commit message?
>
> If you have a SysV Daemon, you should call setsid() to detach from
> any terminal and create an independent session.
>
> If we are running in systemd we should run in forground mode and not
> call setsid()!
>
> See
>
> https://www.freedesktop.org/software/systemd/man/daemon.html
>
> how SysV Daemons should be implemented.

So, I take it that the latest patch set allows SysV to work and
also allows systemd to work 'correctly'

Rowland
 

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
On Tuesday, 28 November 2017 11:19:57 CET Rowland Penny wrote:

> On Tue, 28 Nov 2017 10:59:15 +0100
> Andreas Schneider via samba-technical <[hidden email]>
>
> wrote:
> > On Monday, 27 November 2017 22:33:22 CET Andrew Bartlett via
> >
> > samba-technical wrote:
> > > On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
> > >
> > > technical wrote:
> > > > Hi,
> > > >
> > > > attached is a patch to address an issue with systemd and Samba
> > > > daemons running in notify mode. In this mode we should not double
> > > > fork. So we should start the daemons with --foreground. Also
> > > > systemd will take care of the process group we should not handle
> > > > that in Samba.
> > > >
> > > > For this I've added --foreground to the 'samba' daemon.
> > > >
> > > >
> > > > Review is much appreciated. If OK please push.
> > >
> > > My main question comes from not really understanding what a session
> > > ID is in unix.  Your patch makes --no-process-group also change us
> > > to call getsid(), which was unconditionally false previously.
> > >
> > > Is that reasonable?  If so, can you just write up what exactly it
> > > means in the commit message?
> >
> > If you have a SysV Daemon, you should call setsid() to detach from
> > any terminal and create an independent session.
> >
> > If we are running in systemd we should run in forground mode and not
> > call setsid()!
> >
> > See
> >
> > https://www.freedesktop.org/software/systemd/man/daemon.html
> >
> > how SysV Daemons should be implemented.
>
> So, I take it that the latest patch set allows SysV to work and
> also allows systemd to work 'correctly'

Exactly :-)

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Run 'samba' daemon in foreground

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-11-28 at 10:59 +0100, Andreas Schneider wrote:

> On Monday, 27 November 2017 22:33:22 CET Andrew Bartlett via samba-technical
> wrote:
> > On Mon, 2017-11-27 at 19:43 +0100, Andreas Schneider via samba-
> >
> > technical wrote:
> > > Hi,
> > >
> > > attached is a patch to address an issue with systemd and Samba daemons
> > > running in notify mode. In this mode we should not double fork. So we
> > > should start the daemons with --foreground. Also systemd will take care
> > > of the process group we should not handle that in Samba.
> > >
> > > For this I've added --foreground to the 'samba' daemon.
> > >
> > >
> > > Review is much appreciated. If OK please push.
> >
> > My main question comes from not really understanding what a session ID
> > is in unix.  Your patch makes --no-process-group also change us to call
> > getsid(), which was unconditionally false previously.
> >
> > Is that reasonable?  If so, can you just write up what exactly it means
> > in the commit message?
>
> If you have a SysV Daemon, you should call setsid() to detach from any
> terminal and create an independent session.
>
> If we are running in systemd we should run in forground mode and not call
> setsid()!
>
> See
>
> https://www.freedesktop.org/software/systemd/man/daemon.html
>
> how SysV Daemons should be implemented.
>
> > The options here are a confusing mess, so noting the differences with
> > -i (--interactive) would be helpful.  As I read it, the only difference
> > is that with -i logs got to STDOUT and with --foreground
> > become_daemon() is called, potentially allowing --no-process-group to
> > trigger the setsid()?
>
> No, --no-process-group does NOT trigger setsid()!
>
> samba --no-process-group:
> no_process_group=true  =>  become_daemon(no_session=true)
>
> if (!no_session) {
>   setsid()
> }
>
> Updated patchset attached.

Thanks!  

Reviewed-by: Andrew Bartlett <[hidden email]>

In autobuild now.

Thanks to you and everyone else pitching in for all the recent tidy-up
and portability work.  It is nice to see this improved.

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