[PATCH] Use setproctitle in winbindd

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

[PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
Hi!

Looking at the process list on a Samba DC with all those accurately named samba
processes, I got jealous and wanted the same in winbindd as well.

Patch attached, already reviewed by Andreas, so I'm going to push later if noone
objects.

How does it look like? Eg:

root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

winbindd-setproctitle.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
Hi Ralph,

looks good to me, please push.

Thanks!
metze

Am 09.01.2018 um 11:17 schrieb Ralph Böhme via samba-technical:

> Hi!
>
> Looking at the process list on a Samba DC with all those accurately named samba
> processes, I got jealous and wanted the same in winbindd as well.
>
> Patch attached, already reviewed by Andreas, so I'm going to push later if noone
> objects.
>
> How does it look like? Eg:
>
> root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
> root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
> root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
> root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
> root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
>
> -slow
>


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical wrote:

> Hi!
>
> Looking at the process list on a Samba DC with all those accurately named samba
> processes, I got jealous and wanted the same in winbindd as well.
>
> Patch attached, already reviewed by Andreas, so I'm going to push later if noone
> objects.
>
> How does it look like? Eg:
>
> root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
> root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
> root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
> root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
> root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
>
> -slow

Hi,

i think this is a good idea. Sometimes it is important to identify a
domain child process and currently the only method is checking the open
files of the processes for a log file with the domain name.

Have you looked how other parts of the code set the title? For smbd,
this is done from reinit_after_fork that then passes the last parameter
to prctl_set_comment which then calls prctl(PR_SET_NAME).

It would be good to have a common path to set the process names.

Christof


>
> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From 01761d6ee07fe159ec9c76803bbaa89f789fa133 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Wed, 20 Dec 2017 17:42:45 +0100
> Subject: [PATCH] winbindd: use setproctitle
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> Reviewed-by: Andreas Schneider <[hidden email]>
> ---
>  source3/winbindd/winbindd.c      | 4 ++++
>  source3/winbindd/winbindd_cm.c   | 2 ++
>  source3/winbindd/winbindd_dual.c | 6 ++++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/source3/winbindd/winbindd.c b/source3/winbindd/winbindd.c
> index 460ee4b7ce4..74b240c5af3 100644
> --- a/source3/winbindd/winbindd.c
> +++ b/source3/winbindd/winbindd.c
> @@ -508,6 +508,8 @@ static void winbind_msg_validate_cache(struct messaging_context *msg_ctx,
>   /* install default SIGCHLD handler: validation code uses fork/waitpid */
>   CatchSignal(SIGCHLD, SIG_DFL);
>  
> + setproctitle("validate cache child");
> +
>   ret = (uint8_t)winbindd_validate_cache_nobackup();
>   DEBUG(10, ("winbindd_msg_validata_cache: got return value %d\n", ret));
>   messaging_send_buf(msg_ctx, server_id, MSG_WINBIND_VALIDATE_CACHE, &ret,
> @@ -1482,6 +1484,8 @@ int main(int argc, const char **argv)
>   NTSTATUS status;
>   bool ok;
>  
> + setproctitle_init(argc, discard_const(argv), environ);
> +
>   /*
>   * Do this before any other talloc operation
>   */
> diff --git a/source3/winbindd/winbindd_cm.c b/source3/winbindd/winbindd_cm.c
> index 16836bd05b5..4d3a372dd25 100644
> --- a/source3/winbindd/winbindd_cm.c
> +++ b/source3/winbindd/winbindd_cm.c
> @@ -256,6 +256,8 @@ static bool fork_child_dc_connect(struct winbindd_domain *domain)
>   }
>   SAFE_FREE(lfile);
>  
> + setproctitle("dc-connect child");
> +
>   mem_ctx = talloc_init("fork_child_dc_connect");
>   if (!mem_ctx) {
>   DEBUG(0,("talloc_init failed.\n"));
> diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
> index 3b25e53e786..a05644d2c34 100644
> --- a/source3/winbindd/winbindd_dual.c
> +++ b/source3/winbindd/winbindd_dual.c
> @@ -1491,6 +1491,12 @@ static bool fork_domain_child(struct winbindd_child *child)
>   _exit(0);
>   }
>  
> + if (child_domain != NULL) {
> + setproctitle("domain child [%s]", child_domain->name);
> + } else if (child == idmap_child()) {
> + setproctitle("idmap child");
> + }
> +
>   /* Handle online/offline messages. */
>   messaging_register(server_messaging_context(), NULL,
>     MSG_WINBIND_OFFLINE, child_msg_offline);
> --
> 2.13.6
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
On Tue, 2018-01-09 at 12:07 -0700, Christof Schmitt via samba-technical
wrote:

> On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical wrote:
> > Hi!
> >
> > Looking at the process list on a Samba DC with all those accurately named samba
> > processes, I got jealous and wanted the same in winbindd as well.
> >
> > Patch attached, already reviewed by Andreas, so I'm going to push later if noone
> > objects.
> >
> > How does it look like? Eg:
> >
> > root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
> > root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
> > root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
> > root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
> > root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
> >
> > -slow
>
> Hi,
>
> i think this is a good idea. Sometimes it is important to identify a
> domain child process and currently the only method is checking the open
> files of the processes for a log file with the domain name.
>
> Have you looked how other parts of the code set the title? For smbd,
> this is done from reinit_after_fork that then passes the last parameter
> to prctl_set_comment which then calls
> prctl(PR_SET_NAME).
>
> It would be good to have a common path to set the process names.
>
> Christof

I agree.  A pattern where we set the prctl(PR_SET_NAME) with a
truncated name and the setproctitle with a longer one would be a very
good idea, ideally as a function that calls both, eg
set_samba_process_titles(short, long).

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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tuesday, 9 January 2018 20:07:18 CET Christof Schmitt via samba-technical
wrote:
> On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical
wrote:

> > Hi!
> >
> > Looking at the process list on a Samba DC with all those accurately named
> > samba processes, I got jealous and wanted the same in winbindd as well.
> >
> > Patch attached, already reviewed by Andreas, so I'm going to push later if
> > noone objects.
> >
> > How does it look like? Eg:
> >
> > root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07
> > ./bin/winbindd -D root     31724  0.0  0.7 316200 15152 ?        S  
> > Jan06   0:01  \_ winbindd: domain child [TITAN] root     31727  0.0  0.8
> > 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child
> > [WDOM2] root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01
> > \_ winbindd: idmap child root     31729  0.0  0.4 309044  9004 ?        S
> >    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
> >
> > -slow
>
> Hi,
>
> i think this is a good idea. Sometimes it is important to identify a
> domain child process and currently the only method is checking the open
> files of the processes for a log file with the domain name.
>
> Have you looked how other parts of the code set the title? For smbd,
> this is done from reinit_after_fork that then passes the last parameter
> to prctl_set_comment which then calls prctl(PR_SET_NAME).
>
> It would be good to have a common path to set the process names.

Also lsasd, epmd and spoolssd should set it :-)



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
On Tue, Jan 09, 2018 at 08:11:12PM +0100, Andreas Schneider via samba-technical wrote:

> On Tuesday, 9 January 2018 20:07:18 CET Christof Schmitt via samba-technical
> wrote:
> > On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical
> wrote:
> > > Hi!
> > >
> > > Looking at the process list on a Samba DC with all those accurately named
> > > samba processes, I got jealous and wanted the same in winbindd as well.
> > >
> > > Patch attached, already reviewed by Andreas, so I'm going to push later if
> > > noone objects.
> > >
> > > How does it look like? Eg:
> > >
> > > root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07
> > > ./bin/winbindd -D root     31724  0.0  0.7 316200 15152 ?        S  
> > > Jan06   0:01  \_ winbindd: domain child [TITAN] root     31727  0.0  0.8
> > > 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child
> > > [WDOM2] root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01
> > > \_ winbindd: idmap child root     31729  0.0  0.4 309044  9004 ?        S
> > >    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
> > >
> > > -slow
> >
> > Hi,
> >
> > i think this is a good idea. Sometimes it is important to identify a
> > domain child process and currently the only method is checking the open
> > files of the processes for a log file with the domain name.
> >
> > Have you looked how other parts of the code set the title? For smbd,
> > this is done from reinit_after_fork that then passes the last parameter
> > to prctl_set_comment which then calls prctl(PR_SET_NAME).
> >
> > It would be good to have a common path to set the process names.
>
> Also lsasd, epmd and spoolssd should set it :-)

Oooh yes. This is a significant improvement ! Please push asap.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Jan 10, 2018 at 08:10:17AM +1300, Andrew Bartlett via samba-technical wrote:

> On Tue, 2018-01-09 at 12:07 -0700, Christof Schmitt via samba-technical
> wrote:
> > On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical wrote:
> > > Hi!
> > >
> > > Looking at the process list on a Samba DC with all those accurately named samba
> > > processes, I got jealous and wanted the same in winbindd as well.
> > >
> > > Patch attached, already reviewed by Andreas, so I'm going to push later if noone
> > > objects.
> > >
> > > How does it look like? Eg:
> > >
> > > root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
> > > root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
> > > root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
> > > root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
> > > root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
> > >
> > > -slow
> >
> > Hi,
> >
> > i think this is a good idea. Sometimes it is important to identify a
> > domain child process and currently the only method is checking the open
> > files of the processes for a log file with the domain name.
> >
> > Have you looked how other parts of the code set the title? For smbd,
> > this is done from reinit_after_fork that then passes the last parameter
> > to prctl_set_comment which then calls
> > prctl(PR_SET_NAME).
> >
> > It would be good to have a common path to set the process names.
> >
> > Christof
>
> I agree.  A pattern where we set the prctl(PR_SET_NAME) with a
> truncated name and the setproctitle with a longer one would be a very
> good idea, ideally as a function that calls both, eg
> set_samba_process_titles(short, long).

what for? Why calling prctl? My understanding was calling setproctitle is all
that's needed for fancy ps output across the board. Isn't that what the samba
binary calls as well?

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use setproctitle in winbindd

Samba - samba-technical mailing list
On Tue, 2018-01-09 at 20:54 +0100, Ralph Böhme wrote:

> On Wed, Jan 10, 2018 at 08:10:17AM +1300, Andrew Bartlett via samba-technical wrote:
> >
> > I agree.  A pattern where we set the prctl(PR_SET_NAME) with a
> > truncated name and the setproctitle with a longer one would be a very
> > good idea, ideally as a function that calls both, eg
> > set_samba_process_titles(short, long).
>
> what for? Why calling prctl? My understanding was calling setproctitle is all
> that's needed for fancy ps output across the board. Isn't that what the samba
> binary calls as well?

They are different tags.  The setproctitle thing is via libbsd and is
frankly a totally disgusting hack.  However, it is broadly used enough
that I assume it will keep working.

This describes a little of the approaches:
 https://tycho.ws/blog/2015/02/setproctitle.html

However note that it looks like libbsd doesn't use the
ptrctl(PR_SET_MM) et al.

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] Use setproctitle in winbindd

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jan 09, 2018 at 08:54:45PM +0100, Ralph Böhme wrote:

> On Wed, Jan 10, 2018 at 08:10:17AM +1300, Andrew Bartlett via samba-technical wrote:
> > On Tue, 2018-01-09 at 12:07 -0700, Christof Schmitt via samba-technical
> > wrote:
> > > On Tue, Jan 09, 2018 at 11:17:57AM +0100, Ralph Böhme via samba-technical wrote:
> > > > Hi!
> > > >
> > > > Looking at the process list on a Samba DC with all those accurately named samba
> > > > processes, I got jealous and wanted the same in winbindd as well.
> > > >
> > > > Patch attached, already reviewed by Andreas, so I'm going to push later if noone
> > > > objects.
> > > >
> > > > How does it look like? Eg:
> > > >
> > > > root     31717  0.0  0.9 436076 20028 ?        Ss   Jan06   0:07 ./bin/winbindd -D
> > > > root     31724  0.0  0.7 316200 15152 ?        S    Jan06   0:01  \_ winbindd: domain child [TITAN]
> > > > root     31727  0.0  0.8 436080 18200 ?        S    Jan06   0:00  \_ winbindd: domain child [WDOM2]
> > > > root     31728  0.0  0.9 443576 19112 ?        S    Jan06   0:01  \_ winbindd: idmap child        
> > > > root     31729  0.0  0.4 309044  9004 ?        S    Jan06   0:01  \_ winbindd: domain child [BUILTIN]
> > > >
> > > > -slow
> > >
> > > Hi,
> > >
> > > i think this is a good idea. Sometimes it is important to identify a
> > > domain child process and currently the only method is checking the open
> > > files of the processes for a log file with the domain name.
> > >
> > > Have you looked how other parts of the code set the title? For smbd,
> > > this is done from reinit_after_fork that then passes the last parameter
> > > to prctl_set_comment which then calls
> > > prctl(PR_SET_NAME).
> > >
> > > It would be good to have a common path to set the process names.
> > >
> > > Christof
> >
> > I agree.  A pattern where we set the prctl(PR_SET_NAME) with a
> > truncated name and the setproctitle with a longer one would be a very
> > good idea, ideally as a function that calls both, eg
> > set_samba_process_titles(short, long).
>
> what for? Why calling prctl? My understanding was calling setproctitle is all
> that's needed for fancy ps output across the board. Isn't that what the samba
> binary calls as well?

As Andrew pointed out, argv[0] and the process name (or "comm" in
Linux) are two different names. The situation now for Samba is that
depending on which one is displayed, the naming is inconsistent. See
e.g. this output from a 'make testenv':

$ ps axjf

13506 13566 13484  3698 pts/1    13484 S+    1000   0:00  |               |       \_ samba: root process
13566 13570 13484  3698 pts/1    13484 S+    1000   0:00  |               |       |   \_ samba: tfork waiter process
13570 13571 13571 13571 ?           -1 Ss    1000   0:00  |               |       |   |   \_ /samba/bin/winbindd -D --option=server role check:inhibit=yes --foreground --stdout
13571 13581 13571 13571 ?           -1 S     1000   0:00  |               |       |   |       \_ winbindd: domain child [ADDOMAIN]            .
13571 13585 13571 13571 ?           -1 S     1000   0:00  |               |       |   |       \_ winbindd: idmap child                        .
13571 13586 13571 13571 ?           -1 S     1000   0:00  |               |       |   |       \_ winbindd: domain child [BUILTIN]             .
13566 13578 13484  3698 pts/1    13484 S+    1000   0:00  |               |       |   \_ samba: tfork waiter process
13578 13579 13579 13579 ?           -1 Ss    1000   0:00  |               |       |       \_ /samba/bin/smbd -D --option=server role check:inhibit=yes --foreground --log-stdout
13579 13583 13579 13579 ?           -1 S     1000   0:00  |               |       |           \_ /samba/bin/smbd -D --option=server role check:inhibit=yes --foreground --log-stdout
13579 13584 13579 13579 ?           -1 S     1000   0:00  |               |       |           \_ /samba/bin/smbd -D --option=server role check:inhibit=yes --foreground --log-stdout
13579 13587 13579 13579 ?           -1 S     1000   0:00  |               |       |           \_ /samba/bin/smbd -D --option=server role check:inhibit=yes --foreground --log-stdout

$ ps -ejH

13566 13484  3698 pts/1    00:00:00                 samba
13570 13484  3698 pts/1    00:00:00                   samba
13571 13571 13571 ?        00:00:00                     winbindd
13581 13571 13571 ?        00:00:00                       winbindd
13585 13571 13571 ?        00:00:00                       winbindd
13586 13571 13571 ?        00:00:00                       winbindd
13578 13484  3698 pts/1    00:00:00                   samba
13579 13579 13579 ?        00:00:00                     smbd
13583 13579 13579 ?        00:00:00                       smbd-notifyd
13584 13579 13579 ?        00:00:00                       cleanupd
13587 13579 13579 ?        00:00:00                       lpqd

My proposal would be to have a new function in lib/util/util_process.c that
takes two arguments and tries to set both names. A fallback would be if only
the short name (<= 16 bytes) is provided, that can be used for both cases. Then
this function should be the preferred approach to set the process names. Time
permitting, i will work on a patch.

Christof