coredump: use current->group_leader->comm instead of current->comm

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.

--

@@ -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;
 			}

Which might break stuff.

Which some people may well be using

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

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.

--

@@ -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':

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 ?

Indeed. The important case is the private/synchronous signals like SIGSEGV, you can see the name of the thread which triggered the crash.

Oleg,

Which speaks partly to my notion of "consistency".

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)

While that is true, it doesn't seem to have been the original intent as per the %e documentation.

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

That is why I mentioned it with "otoh" ;)

May be. May be not. I do not know.

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.

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.

Oleg, Alan,

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.

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