[PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Paul E. McKenney]

This patch appends the localversion string to the Linux kernel version.
For example, in a git tree with uncommitted changes, the .config file
might start as follows (but with leading hash marks):

	Automatically generated make config: don't edit
	Linux kernel version: 2.6.33-01836-g90a6501-dirty
	Mon Mar  1 17:05:59 2010

The "-01836-g90a6501-dirty" string is added by this patch.

The KBUILD_CONFIG_NO_CHECK_DIRTY environment variable controls the
git "-dirty" check.  If this variable is either empty or undefined,
then a "-dirty" check is performed (the default), otherwise, this
check is omitted.

Differences from v2:

o	Replace popen() with the equivalent fork-exec series
	to prevent security vulnerabilities due to shell metacharacter
	interpretation.

o	Added the KBUILD_CONFIG_NO_CHECK_DIRTY environment variable,
	and modified scripts/setlocalversion to check it, as suggested
	by James Cloos.

Differences from v1:

o	Incorporates feedback from Geert Uytterhoeven, Linus Torvalds,
	Frans Pop, and James Cloos.

o	Fixed to work correctly with the "O=" Makefile argument and
	the KBUILD_OUTPUT environment variable, so that .config files
	created in directories outside of the source tree are tagged
	correctly.

o	Uses scripts/setlocalversion, which handles not only git, but
	also mercurial and svn.

o	Make the new behavior default-off, as scripts/setlocalversion
	has significant latency.  A new environment variable named
	"KBUILD_CONFIG_LOCALVERSION" must be set to enable the
	"-01836-g90a6501-dirty" style of string.

	This is intended to address James Cloos's concern that this
	feature will slow down casual kernel builds.

It has been suggested that this string be output at boot and oops time.
If there is general agreement, this will be the subject of a separate
patch.

Suggested-by: Ingo Molnar <mingo@elte.hu>
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Suggested-by: Frans Pop <elendil@planet.nl>
Suggested-by: James Cloos <cloos@jhcloos.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Frans Pop <elendil@planet.nl>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 kconfig/confdata.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 setlocalversion    |   17 ++++++++++-------
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4dec80..e896b81 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -15,6 +15,8 @@
 #define LKC_DIRECT_LINK
 #include "lkc.h"
 
+extern char **environ;
+
 static void conf_warning(const char *fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
 
@@ -399,15 +401,20 @@ int conf_read(const char *name)
 int conf_write(const char *name)
 {
 	FILE *out;
+	FILE *slv;
 	struct symbol *sym;
 	struct menu *menu;
 	const char *basename;
-	char dirname[128], tmpname[128], newname[128];
+	char dirname[128], tmpname[128], newname[128], localversion[128];
+	char cmdline[PATH_MAX * 2 + 128];
 	int type, l;
 	const char *str;
 	time_t now;
 	int use_timestamp = 1;
+	int pipefd[2];
 	char *env;
+	char *path;
+	pid_t pid;
 
 	dirname[0] = 0;
 	if (name && name[0]) {
@@ -450,12 +457,52 @@ int conf_write(const char *name)
 	if (env && *env)
 		use_timestamp = 0;
 
+	strcpy(localversion, "-?-nopath");
+	path = getenv(SRCTREE);
+	if (path && *path) {
+		strcpy(localversion, "-?-pipe()-failed");
+		if (pipe(pipefd) != 0)
+			goto nolocalversion;
+		env = getenv("KBUILD_CONFIG_NO_CHECK_DIRTY");
+		sprintf(cmdline, "%s/scripts/setlocalversion", path);
+		strcpy(localversion, "-?-fork()-failed");
+		pid = fork();
+		if (pid < 0)
+			goto nolocalversion;
+		if (pid == 0) { /* child */
+			int fd_new_stderr;
+			char *newargv[] = { cmdline, path, NULL };
+
+			close(1); /* stdout */
+			close(2); /* stderr */
+			fd_new_stderr = open("/dev/null", O_RDONLY);
+			if (dup2(pipefd[1], 1) < 0)
+				_exit(1);
+			if (fd_new_stderr != 2)
+				if (dup2(fd_new_stderr, 1) < 0) {
+					_exit(2);
+			}
+			execve(cmdline, newargv, environ);
+			_exit(3);
+		} else { /* parent */
+			strcpy(localversion, "-?-fscanf()-failed");
+			slv = fdopen(pipefd[0], "r");
+			if (slv != NULL) {
+				close(pipefd[1]);
+				fscanf(slv, " %127s ", localversion);
+				fclose(slv);
+			}
+		}
+	}
+nolocalversion:
+
 	fprintf(out, _("#\n"
 		       "# Automatically generated make config: don't edit\n"
-		       "# Linux kernel version: %s\n"
+		       "# Linux kernel version: %s%s\n"
 		       "%s%s"
 		       "#\n"),
 		     sym_get_string_value(sym),
+		     localversion[0] != '\0' ? localversion : "",
 		     use_timestamp ? "# " : "",
 		     use_timestamp ? ctime(&now) : "");
 
diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 46989b8..3d4ff84 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -39,13 +39,16 @@ if head=`git rev-parse --verify --short HEAD 2>/dev/null`; then
 	        printf -- '-svn%s' "`git svn find-rev $head`"
 	fi
 
-	# Update index only on r/w media
-	[ -w . ] && git update-index --refresh --unmerged > /dev/null
-
-	# Check for uncommitted changes
-	if git diff-index --name-only HEAD | grep -v "^scripts/package" \
-	    | read dummy; then
-		printf '%s' -dirty
+	if [ -z "$KBUILD_CONFIG_NO_CHECK_DIRTY" ]; then
+		# Update index only on r/w media
+		[ -w . ] && git update-index --refresh --unmerged > /dev/null
+
+		# Check for uncommitted changes
+		if git diff-index --name-only HEAD \
+		    | grep -v "^scripts/package" \
+		    | read dummy; then
+			printf '%s' -dirty
+		fi
 	fi
 
 	# All done with git

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Frans Pop]

On Friday 05 March 2010, Paul E. McKenney wrote:
> o	Added the KBUILD_CONFIG_NO_CHECK_DIRTY environment variable,
> 	and modified scripts/setlocalversion to check it, as suggested
> 	by James Cloos.

Just to state the obvious: this will also affect CONFIG_LOCALVERSION_AUTO.

> @@ -450,12 +457,52 @@ int conf_write(const char *name)
>  	if (env && *env)
>  		use_timestamp = 0;
>
> +	strcpy(localversion, "-?-nopath");
> +	path = getenv(SRCTREE);
> +	if (path && *path) {
> +		strcpy(localversion, "-?-pipe()-failed");
> +		if (pipe(pipefd) != 0)
> +			goto nolocalversion;
> +		env = getenv("KBUILD_CONFIG_NO_CHECK_DIRTY");

Is this line actually needed? AFAICT the variable is unused here and should 
pass down through the environment to the setlocalversion script without 
needing any help.

> +		sprintf(cmdline, "%s/scripts/setlocalversion", path);
> +		strcpy(localversion, "-?-fork()-failed");
> +		pid = fork();

Do I read correctly that you're also postfixing error conditions to the 
kernel version? Don't think that's a great idea TBH. Errors should be 
printed to STDERR as they occur, not as pseudo version strings.

Users coming across them in config files would be very unlikely to be able 
to make any sense of them. IMO, if no VCS version can be determined, 
nothing should be printed.

Cheers,
FJP

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Paul E. McKenney]

On Fri, Mar 05, 2010 at 04:43:27AM +0100, Frans Pop wrote:
> On Friday 05 March 2010, Paul E. McKenney wrote:
> > o	Added the KBUILD_CONFIG_NO_CHECK_DIRTY environment variable,
> > 	and modified scripts/setlocalversion to check it, as suggested
> > 	by James Cloos.
> 
> Just to state the obvious: this will also affect CONFIG_LOCALVERSION_AUTO.
> 
> > @@ -450,12 +457,52 @@ int conf_write(const char *name)
> >  	if (env && *env)
> >  		use_timestamp = 0;
> >
> > +	strcpy(localversion, "-?-nopath");
> > +	path = getenv(SRCTREE);
> > +	if (path && *path) {
> > +		strcpy(localversion, "-?-pipe()-failed");
> > +		if (pipe(pipefd) != 0)
> > +			goto nolocalversion;
> > +		env = getenv("KBUILD_CONFIG_NO_CHECK_DIRTY");
> 
> Is this line actually needed? AFAICT the variable is unused here and should 
> pass down through the environment to the setlocalversion script without 
> needing any help.

No, it is not needed.  This is a holdover from an earlier version that
tried passing in just that one environment variable.

Good eyes, will fix!

> > +		sprintf(cmdline, "%s/scripts/setlocalversion", path);
> > +		strcpy(localversion, "-?-fork()-failed");
> > +		pid = fork();
> 
> Do I read correctly that you're also postfixing error conditions to the 
> kernel version? Don't think that's a great idea TBH. Errors should be 
> printed to STDERR as they occur, not as pseudo version strings.

That is the intent.

> Users coming across them in config files would be very unlikely to be able 
> to make any sense of them. IMO, if no VCS version can be determined, 
> nothing should be printed.

Hmmm...  I guess I can leave stderr untouched through to the child and
add the code needed to reap the children and report any error codes.

But let's work out what the error strategy should be.  The below are my
initial guesses, I of course must defer to those more familiar with
kbuild and kconfig than am I.

1.	Oddball SCM conditions should not cause the build to fail.
	"Arrrgh!!!  What dot-file do I need to remove in order for
	my builds to start succeeding???"

2.	Errors should leave some hint in the .config file, rather
	than simply mysteriously omitting the version/dirty information.

	Yes, stderr can be captured, but if it doesn't break the build,
	it is likely to be ignored.

3.	It is OK to dump to stderr (I think), but the format should be
	something that typical error-check scripts catch.

	What would that be?  How about the following pattern that every
	build-error-check script must pay attention to?

	scripts/confdata.c:nnnn error: <perror output>

4.	Should the splat in the .config file identify the file and
	line number?  For example: "-error: scripts/confdata.c:nnnn"

5.	Anything else that I have missed?

After this is done, I am going to return to something easier to
understand, like the Linux kernel's RCU implementation.  ;-)

							Thanx, Paul

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Mike Galbraith]

On Thu, 2010-03-04 at 18:54 -0800, Paul E. McKenney wrote:
> This patch appends the localversion string to the Linux kernel version.
> For example, in a git tree with uncommitted changes, the .config file
> might start as follows (but with leading hash marks):
> 
> 	Automatically generated make config: don't edit
> 	Linux kernel version: 2.6.33-01836-g90a6501-dirty
> 	Mon Mar  1 17:05:59 2010
> 
> The "-01836-g90a6501-dirty" string is added by this patch.

I really like this.  This gives me the advantages of non-volatile build
info without the mess that makes if it's appended to file names/paths.

	-Mike

Eventually someone will make bisect stop stepping on random kernels, and
generally making a mess, and life will be grand ;-)

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Paul E. McKenney]

On Fri, Mar 05, 2010 at 07:10:48AM +0100, Mike Galbraith wrote:
> On Thu, 2010-03-04 at 18:54 -0800, Paul E. McKenney wrote:
> > This patch appends the localversion string to the Linux kernel version.
> > For example, in a git tree with uncommitted changes, the .config file
> > might start as follows (but with leading hash marks):
> > 
> > 	Automatically generated make config: don't edit
> > 	Linux kernel version: 2.6.33-01836-g90a6501-dirty
> > 	Mon Mar  1 17:05:59 2010
> > 
> > The "-01836-g90a6501-dirty" string is added by this patch.
> 
> I really like this.  This gives me the advantages of non-volatile build
> info without the mess that makes if it's appended to file names/paths.

Glad you like it!  ;-)

> 	-Mike
> 
> Eventually someone will make bisect stop stepping on random kernels, and
> generally making a mess, and life will be grand ;-)

Heck, I don't even know how bisect deals with merges.  ;-)

							Thanx, Paul

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Frans Pop]

On Friday 05 March 2010, Paul E. McKenney wrote:
> But let's work out what the error strategy should be.  The below are my
> initial guesses, I of course must defer to those more familiar with
> kbuild and kconfig than am I.

That's not me either :-)
I see you've not CCed linux-kbuild@vger.k.o so far. Suggest you add them 
with the next version.

> 1.	Oddball SCM conditions should not cause the build to fail.
> 	"Arrrgh!!!  What dot-file do I need to remove in order for
> 	my builds to start succeeding???"

Agreed.

> 2.	Errors should leave some hint in the .config file, rather
> 	than simply mysteriously omitting the version/dirty information.

I don't see why this should be treated any different than 
CONFIG_LOCALVERSION_AUTO. Either setlocalversion returns something (on 
stdout) and you use it, or it returns nothing and you don't.

With CONFIG_LOCALVERSION_AUTO errors get ignored (tested by adding 'exit 1' 
early in the script) and output to stderr simply gets displayed (without 
any real identification where it comes from).

If users expect the SCM version info to be there and it isn't, they will 
investigate.

> 4.      Should the splat in the .config file identify the file and
>         line number?  For example: "-error: scripts/confdata.c:nnnn"

IMHO definitely not. I think you're over-designing this. It's not really 
core functionality. My viewpoint is simple: a version string should 
contain version info, and nothing else.

> After this is done, I am going to return to something easier to
> understand, like the Linux kernel's RCU implementation.  ;-)

:-)

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Paul E. McKenney]

On Fri, Mar 05, 2010 at 06:18:08PM +0100, Frans Pop wrote:
> On Friday 05 March 2010, Paul E. McKenney wrote:
> > But let's work out what the error strategy should be.  The below are my
> > initial guesses, I of course must defer to those more familiar with
> > kbuild and kconfig than am I.
> 
> That's not me either :-)
> I see you've not CCed linux-kbuild@vger.k.o so far. Suggest you add them 
> with the next version.

Ah, will do on v5.

> > 1.	Oddball SCM conditions should not cause the build to fail.
> > 	"Arrrgh!!!  What dot-file do I need to remove in order for
> > 	my builds to start succeeding???"
> 
> Agreed.
> 
> > 2.	Errors should leave some hint in the .config file, rather
> > 	than simply mysteriously omitting the version/dirty information.
> 
> I don't see why this should be treated any different than 
> CONFIG_LOCALVERSION_AUTO. Either setlocalversion returns something (on 
> stdout) and you use it, or it returns nothing and you don't.
> 
> With CONFIG_LOCALVERSION_AUTO errors get ignored (tested by adding 'exit 1' 
> early in the script) and output to stderr simply gets displayed (without 
> any real identification where it comes from).
> 
> If users expect the SCM version info to be there and it isn't, they will 
> investigate.

Understood, but I am concerned about the case where one person creates
the configuration and another is looking at the .config file.

> > 4.      Should the splat in the .config file identify the file and
> >         line number?  For example: "-error: scripts/confdata.c:nnnn"
> 
> IMHO definitely not. I think you're over-designing this. It's not really 
> core functionality. My viewpoint is simple: a version string should 
> contain version info, and nothing else.

s/you're over-designing this/you are freaking paranoid/

With that change, I plead guilty to charges as read.  But again, I am
worried about the case where one person generates the .config file
and someone else is reading it.  And my paranoia has proven quite useful
over the years.  ;-)

							Thanx, Paul

> > After this is done, I am going to return to something easier to
> > understand, like the Linux kernel's RCU implementation.  ;-)
> 
> :-)

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Frans Pop]

(Dropping cloos as that address is bouncing my mails.)

On Friday 05 March 2010, Paul E. McKenney wrote:
> s/you're over-designing this/you are freaking paranoid/

Whatever makes you happy :-)

> With that change, I plead guilty to charges as read.  But again, I am
> worried about the case where one person generates the .config file
> and someone else is reading it.

Not sure that's very relevant. A person reading it will still get the same 
oldfashioned basic info as when a build is done from a tree that's not 
under an SCM. And he won't really be in a position where he can fix the 
errors anyway.

Re: [PATCH] v3 kconfig: place git SHA1 in .config output if in SCM

[Paul E. McKenney]

On Fri, Mar 05, 2010 at 09:31:10PM +0100, Frans Pop wrote:
> (Dropping cloos as that address is bouncing my mails.)
> 
> On Friday 05 March 2010, Paul E. McKenney wrote:
> > s/you're over-designing this/you are freaking paranoid/
> 
> Whatever makes you happy :-)

;-)

> > With that change, I plead guilty to charges as read.  But again, I am
> > worried about the case where one person generates the .config file
> > and someone else is reading it.
> 
> Not sure that's very relevant. A person reading it will still get the same 
> oldfashioned basic info as when a build is done from a tree that's not 
> under an SCM. And he won't really be in a position where he can fix the 
> errors anyway.

But the person reading it is in a position to ask the person sending the
.config to fix their build system.

							Thanx, Paul