* [ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm
@ 2011-09-01 17:01 Earl Chew
2011-09-01 18:55 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Earl Chew @ 2011-09-01 17:01 UTC (permalink / raw)
To: linux-kernel; +Cc: alan, viro, andi, oleg
Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.
Multithreaded processes can use PR_SET_NAME (or the corresponding pthread_setname_np)
to configure the name of each thread.
A core dump can be triggered from any task in a group, and substituting current->comm
means that the name of the core file for a process can vary in surprising ways.
Using current->group_leader->comm makes the name of the core file more consistent
for a process when used with %e or %E.
Signed-off-by: Earl Chew <echew@ixiacom.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Oleg Nesterov <oleg@redhat.com>
--
--- fs/exec.c.orig 2011-08-28 21:16:01.000000000 -0700
+++ fs/exec.c 2011-09-01 09:58:42.032116998 -0700
@@ -1679,7 +1679,8 @@ static int cn_print_exe_file(struct core
exe_file = get_mm_exe_file(current->mm);
if (!exe_file) {
char *commstart = cn->corename + cn->used;
- ret = cn_printf(cn, "%s (path unknown)", current->comm);
+ ret = cn_printf(cn, "%s (path unknown)",
+ current->group_leader->comm);
cn_escape(commstart);
return ret;
}
@@ -1780,7 +1781,8 @@ static int format_corename(struct core_n
/* executable */
case 'e': {
char *commstart = cn->corename + cn->used;
- err = cn_printf(cn, "%s", current->comm);
+ err = cn_printf(cn, "%s",
+ current->group_leader->comm);
cn_escape(commstart);
break;
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm
2011-09-01 17:01 [ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm Earl Chew
@ 2011-09-01 18:55 ` Alan Cox
2011-09-01 19:12 ` [PATCH 1/1 v2]: " Earl Chew
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2011-09-01 18:55 UTC (permalink / raw)
To: Earl Chew; +Cc: linux-kernel, viro, andi, oleg
On Thu, 1 Sep 2011 10:01:39 -0700
Earl Chew <echew@ixiacom.com> wrote:
> Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.
Which might break stuff.
> A core dump can be triggered from any task in a group, and substituting current->comm
> means that the name of the core file for a process can vary in surprising ways.
Which some people may well be using
> Using current->group_leader->comm makes the name of the core file more consistent
> for a process when used with %e or %E.
In your view, but there is a better way to do this - add a new case and
letter for the behaviour you want. That way you don't break anyone elses
defaults and expectation and people can set a corepattern dependant upon
the group leader.
Alan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm
2011-09-01 18:55 ` Alan Cox
@ 2011-09-01 19:12 ` Earl Chew
2011-09-02 16:30 ` Oleg Nesterov
0 siblings, 1 reply; 7+ messages in thread
From: Earl Chew @ 2011-09-01 19:12 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, viro, andi, oleg
> In your view, but there is a better way to do this - add a new case and
> letter for the behaviour you want. That way you don't break anyone elses
> defaults and expectation and people can set a corepattern dependant upon
> the group leader.
Ok.
The patterns %n or %N are the same as %e and %E except that they
use current->group_leader->comm instead of current->comm.
Signed-off-by: Earl Chew <echew@ixiacom.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Oleg Nesterov <oleg@redhat.com>
--
--- fs/exec.c.orig 2011-08-28 21:16:01.000000000 -0700
+++ fs/exec.c 2011-09-01 12:03:50.622059092 -0700
@@ -1670,7 +1670,7 @@ static void cn_escape(char *str)
*str = '!';
}
-static int cn_print_exe_file(struct core_name *cn)
+static int cn_print_exe_file(struct core_name *cn, const char *comm)
{
struct file *exe_file;
char *pathbuf, *path;
@@ -1679,7 +1679,7 @@ static int cn_print_exe_file(struct core
exe_file = get_mm_exe_file(current->mm);
if (!exe_file) {
char *commstart = cn->corename + cn->used;
- ret = cn_printf(cn, "%s (path unknown)", current->comm);
+ ret = cn_printf(cn, "%s (path unknown)", comm);
cn_escape(commstart);
return ret;
}
@@ -1777,6 +1777,18 @@ static int format_corename(struct core_n
cn_escape(namestart);
break;
}
+ /* task group executable */
+ case 'n': {
+ char *commstart = cn->corename + cn->used;
+ err = cn_printf(cn, "%s",
+ current->group_leader->comm);
+ cn_escape(commstart);
+ break;
+ }
+ case 'N':
+ err = cn_print_exe_file(cn,
+ current->group_leader->comm);
+ break;
/* executable */
case 'e': {
char *commstart = cn->corename + cn->used;
@@ -1785,7 +1797,7 @@ static int format_corename(struct core_n
break;
}
case 'E':
- err = cn_print_exe_file(cn);
+ err = cn_print_exe_file(cn, current->comm);
break;
/* core limit size */
case 'c':
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm
2011-09-01 19:12 ` [PATCH 1/1 v2]: " Earl Chew
@ 2011-09-02 16:30 ` Oleg Nesterov
2011-09-02 17:09 ` Earl Chew
0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2011-09-02 16:30 UTC (permalink / raw)
To: Earl Chew; +Cc: Alan Cox, linux-kernel, viro, andi
On 09/01, Earl Chew wrote:
>
> > In your view, but there is a better way to do this - add a new case and
> > letter for the behaviour you want. That way you don't break anyone elses
> > defaults and expectation and people can set a corepattern dependant upon
> > the group leader.
>
> Ok.
>
>
> The patterns %n or %N are the same as %e and %E except that they
> use current->group_leader->comm instead of current->comm.
I simply do not know what is better. Alan has a point imho, "might
break stuff" is true.
OTOH, %p always reports tgid, not tid...
But in fact I do not understand the "Using current->group_leader->comm
makes the name of the core file more consistent" part. Why ?
> A core dump can be triggered from any task in a group,
Indeed. The important case is the private/synchronous signals like
SIGSEGV, you can see the name of the thread which triggered the crash.
> -static int cn_print_exe_file(struct core_name *cn)
> +static int cn_print_exe_file(struct core_name *cn, const char *comm)
> {
> struct file *exe_file;
> char *pathbuf, *path;
> @@ -1679,7 +1679,7 @@ static int cn_print_exe_file(struct core
> exe_file = get_mm_exe_file(current->mm);
> if (!exe_file) {
> char *commstart = cn->corename + cn->used;
> - ret = cn_printf(cn, "%s (path unknown)", current->comm);
> + ret = cn_printf(cn, "%s (path unknown)", comm);
Imho, this is overkill. This is only used if get_mm_exe_file() fails,
I don't think this deserves another option. And may be we can use
group_leader->comm, this is per-process thing anyway.
But I won't insist, I agree either way.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm
2011-09-02 16:30 ` Oleg Nesterov
@ 2011-09-02 17:09 ` Earl Chew
2011-09-02 17:48 ` Oleg Nesterov
0 siblings, 1 reply; 7+ messages in thread
From: Earl Chew @ 2011-09-02 17:09 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Alan Cox, linux-kernel, viro, andi
Oleg,
>> The patterns %n or %N are the same as %e and %E except that they
>> use current->group_leader->comm instead of current->comm.
>
> I simply do not know what is better. Alan has a point imho, "might
> break stuff" is true.
>
> OTOH, %p always reports tgid, not tid...
Which speaks partly to my notion of "consistency".
> But in fact I do not understand the "Using current->group_leader->comm
> makes the name of the core file more consistent" part. Why ?
Internals aside, "%e" is advertised, rightly or wrongly as:
http://lxr.linux.no/#linux+v3.0.4/Documentation/sysctl/kernel.txt
%e executable filename (may be shortened)
Using current->comm has the following issues with respect to
the documentation :
Issue 1. The executable filename has the attribute that it is
the same for all threads in one process, while current->comm
does not.
Issue 2. Even for the group leader current->comm is not guaranteed to
be the executable filename at all (the new %E yields that).
I viewed my original change as more "consistent" because it
yielded the attribute alluded to in the documentation --- the
same value for all threads in the one process:
- Consistent with the documentation
- Consistent with respect to process name (as opposed to thread name)
>> A core dump can be triggered from any task in a group,
>
> Indeed. The important case is the private/synchronous signals like
> SIGSEGV, you can see the name of the thread which triggered the crash.
While that is true, it doesn't seem to have been the original intent as
per the %e documentation.
> Imho, this is overkill. This is only used if get_mm_exe_file() fails,
> I don't think this deserves another option. And may be we can use
> group_leader->comm, this is per-process thing anyway.
>
> But I won't insist, I agree either way.
Fundamentally, which do you consider "broken" ?
o fs/exec.c using current->comm for %e
or
o Documentation/sysctl/kernel.txt
I suspect we'll fall on the side of keeping "broken behaviour" since
it affects existing code, and instead fix the documentation.
Should get_mm_exe_file() just use current->group_leader->comm since it's
meant to be process specific anyway, and there isn't an existing code base
for %E ?
Evidently I need to patch Documentation/sysctl/kernel.txt too.
Earl
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm
2011-09-02 17:09 ` Earl Chew
@ 2011-09-02 17:48 ` Oleg Nesterov
2011-09-02 23:05 ` Earl Chew
0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2011-09-02 17:48 UTC (permalink / raw)
To: Earl Chew; +Cc: Alan Cox, linux-kernel, viro, andi
On 09/02, Earl Chew wrote:
>
> Oleg,
>
> >> The patterns %n or %N are the same as %e and %E except that they
> >> use current->group_leader->comm instead of current->comm.
> >
> > I simply do not know what is better. Alan has a point imho, "might
> > break stuff" is true.
> >
> > OTOH, %p always reports tgid, not tid...
>
> Which speaks partly to my notion of "consistency".
That is why I mentioned it with "otoh" ;)
> I viewed my original change as more "consistent" because it
> yielded the attribute alluded to in the documentation --- the
> same value for all threads in the one process:
>
> - Consistent with the documentation
> - Consistent with respect to process name (as opposed to thread name)
>
> >> A core dump can be triggered from any task in a group,
> >
> > Indeed. The important case is the private/synchronous signals like
> > SIGSEGV, you can see the name of the thread which triggered the crash.
>
> While that is true, it doesn't seem to have been the original intent as
> per the %e documentation.
May be. May be not. I do not know.
> Should get_mm_exe_file() just use current->group_leader->comm since it's
> meant to be process specific anyway,
Probably. Although group_leader->comm is thread specific too, but
at least we do not use the "random" thread. My only point was, imho
this doesn't deserve another option.
> and there isn't an existing code base
> for %E ?
Who knows? But once again, we use ->comm in the very unlikely case.
And let me repeat just in case. I do not argue, I agree either way.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 v2]: coredump: use current->group_leader->comm instead of current->comm
2011-09-02 17:48 ` Oleg Nesterov
@ 2011-09-02 23:05 ` Earl Chew
0 siblings, 0 replies; 7+ messages in thread
From: Earl Chew @ 2011-09-02 23:05 UTC (permalink / raw)
To: Oleg Nesterov, Alan Cox; +Cc: linux-kernel, viro, andi
Oleg, Alan,
> Who knows? But once again, we use ->comm in the very unlikely case.
>
> And let me repeat just in case. I do not argue, I agree either way.
I understand.
Before I give thought to reworking the change, perhaps some other
considerations.
The other interesting thing is that Alan brought up the relevant point
about stuff breaking.
Alan Cox wrote:
> Earl wrote:
>> Change corepattern %e and %E to use current->group_leader->comm instead of current->comm.
>
> Which might break stuff.
What are your thoughts regarding the introduction of cn_escape() in this patch ?
https://lkml.org/lkml/2011/6/7/292
While this change also has merit, it is also breaking change. There might
be code relying on %e containing embedded slashes, either directly in
core pattern:
%e.core
or via the pipe:
| /opt/corehandler '%e'
Isn't the interface change introduced by cn_escape() also a concern ?
Earl
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-09-02 23:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-01 17:01 [ PATCH 1/1 ] coredump: use current->group_leader->comm instead of current->comm Earl Chew
2011-09-01 18:55 ` Alan Cox
2011-09-01 19:12 ` [PATCH 1/1 v2]: " Earl Chew
2011-09-02 16:30 ` Oleg Nesterov
2011-09-02 17:09 ` Earl Chew
2011-09-02 17:48 ` Oleg Nesterov
2011-09-02 23:05 ` Earl Chew
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.