[PATCH 1/2] kconfig: do not accept a directory for configuration output

[PATCH 1/2] kconfig: do not accept a directory for configuration output

[Masahiro Yamada]

Currently, conf_write() can be called with a directory name instead
of a file name. As far as I see, this can happen for menuconfig,
nconfig, gconfig.

If it is given with a directory path, conf_write() kindly appends
getenv("KCONFIG_CONFIG"), but this ends up with hacky dir/basename
handling, and screwed up with a corner-case like "what if
KCONFIG_CONFIG is an absolute path?" as discussed before:

  https://patchwork.kernel.org/patch/9910037/

Since conf_write() is already messed up, I'd say "do not do it".
Please pass a file path all the time. If a directory path is specified
for the configuration output, conf_write() will simply error out.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Nicolas Porcel <nicolasporcel06@gmail.com>
---

 scripts/kconfig/confdata.c | 58 ++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 08ba146a83c5..9fd6430c93d2 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -817,40 +817,31 @@ int conf_write(const char *name)
 	FILE *out;
 	struct symbol *sym;
 	struct menu *menu;
-	const char *basename;
 	const char *str;
-	char dirname[PATH_MAX+1], tmpname[PATH_MAX+22], newname[PATH_MAX+8];
+	char tmpname[PATH_MAX + 1], oldname[PATH_MAX + 1];
 	char *env;
 
-	dirname[0] = 0;
-	if (name && name[0]) {
-		char *slash;
-
-		if (is_dir(name)) {
-			strcpy(dirname, name);
-			strcat(dirname, "/");
-			basename = conf_get_configname();
-		} else if ((slash = strrchr(name, '/'))) {
-			int size = slash - name + 1;
-			memcpy(dirname, name, size);
-			dirname[size] = 0;
-			if (slash[1])
-				basename = slash + 1;
-			else
-				basename = conf_get_configname();
-		} else
-			basename = name;
-	} else
-		basename = conf_get_configname();
-
-	sprintf(newname, "%s%s", dirname, basename);
+	if (!name)
+		name = conf_get_configname();
+
+	if (!*name) {
+		fprintf(stderr, "config name is empty\n");
+		return -1;
+	}
+
+	if (is_dir(name)) {
+		fprintf(stderr, "%s: Is a directory\n", name);
+		return -1;
+	}
+
 	env = getenv("KCONFIG_OVERWRITECONFIG");
-	if (!env || !*env) {
-		sprintf(tmpname, "%s.tmpconfig.%d", dirname, (int)getpid());
-		out = fopen(tmpname, "w");
-	} else {
+	if (env && *env) {
 		*tmpname = 0;
-		out = fopen(newname, "w");
+		out = fopen(name, "w");
+	} else {
+		snprintf(tmpname, sizeof(tmpname), "%s.%d.tmp",
+			 name, (int)getpid());
+		out = fopen(tmpname, "w");
 	}
 	if (!out)
 		return 1;
@@ -897,14 +888,13 @@ int conf_write(const char *name)
 	fclose(out);
 
 	if (*tmpname) {
-		strcat(dirname, basename);
-		strcat(dirname, ".old");
-		rename(newname, dirname);
-		if (rename(tmpname, newname))
+		snprintf(oldname, sizeof(oldname), "%s.old", name);
+		rename(name, oldname);
+		if (rename(tmpname, name))
 			return 1;
 	}
 
-	conf_message("configuration written to %s", newname);
+	conf_message("configuration written to %s", name);
 
 	sym_set_change_count(0);
 
-- 
2.17.1

[PATCH 2/2] kconfig: do not write .config if the content is the same

[Masahiro Yamada]

Kconfig updates the .config when it exits even if its content is
exactly the same as before. Since its timestamp becomes newer than
that of other build artifacts, additional processing is invoked,
which is annoying.

- syncconfig is invoked to update include/config/auto.conf, etc.

- kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
  then vmlinux is relinked as well.

If the .config is not changed at all, we do not have to even
touch it. Just bail out showing "No change to .config".

  $ make allmodconfig
  scripts/kconfig/conf  --allmodconfig Kconfig
  #
  # configuration written to .config
  #
  $ make allmodconfig
  scripts/kconfig/conf  --allmodconfig Kconfig
  #
  # No change to .config
  #

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 scripts/kconfig/confdata.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 9fd6430c93d2..399973e35533 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2002 Roman Zippel <zippel@linux-m68k.org>
  */
 
+#include <sys/mman.h>
 #include <sys/stat.h>
 #include <ctype.h>
 #include <errno.h>
@@ -36,6 +37,52 @@ static bool is_dir(const char *path)
 	return S_ISDIR(st.st_mode);
 }
 
+/* return true if the given two files are the same, false otherwise */
+static bool is_same(const char *file1, const char *file2)
+{
+	int fd1, fd2;
+	struct stat st1, st2;
+	void *map1, *map2;
+	bool ret = false;
+
+	fd1 = open(file1, O_RDONLY);
+	if (fd1 < 0)
+		return ret;
+
+	fd2 = open(file2, O_RDONLY);
+	if (fd2 < 0)
+		goto close1;
+
+	ret = fstat(fd1, &st1);
+	if (ret)
+		goto close2;
+	ret = fstat(fd2, &st2);
+	if (ret)
+		goto close2;
+
+	if (st1.st_size != st2.st_size)
+		goto close2;
+
+	map1 = mmap(NULL, st1.st_size, PROT_READ, MAP_PRIVATE, fd1, 0);
+	if (map1 == MAP_FAILED)
+		goto close2;
+
+	map2 = mmap(NULL, st2.st_size, PROT_READ, MAP_PRIVATE, fd2, 0);
+	if (map2 == MAP_FAILED)
+		goto close2;
+
+	if (bcmp(map1, map2, st1.st_size))
+		goto close2;
+
+	ret = true;
+close2:
+	close(fd2);
+close1:
+	close(fd1);
+
+	return ret;
+}
+
 /*
  * Create the parent directory of the given path.
  *
@@ -888,6 +935,13 @@ int conf_write(const char *name)
 	fclose(out);
 
 	if (*tmpname) {
+		if (is_same(name, tmpname)) {
+			conf_message("No change to %s", name);
+			unlink(tmpname);
+			sym_set_change_count(0);
+			return 0;
+		}
+
 		snprintf(oldname, sizeof(oldname), "%s.old", name);
 		rename(name, oldname);
 		if (rename(tmpname, name))
-- 
2.17.1

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Greg KH]

On Fri, May 10, 2019 at 03:12:05PM +0900, Masahiro Yamada wrote:
> Kconfig updates the .config when it exits even if its content is
> exactly the same as before. Since its timestamp becomes newer than
> that of other build artifacts, additional processing is invoked,
> which is annoying.
> 
> - syncconfig is invoked to update include/config/auto.conf, etc.
> 
> - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
>   then vmlinux is relinked as well.
> 
> If the .config is not changed at all, we do not have to even
> touch it. Just bail out showing "No change to .config".
> 
>   $ make allmodconfig
>   scripts/kconfig/conf  --allmodconfig Kconfig
>   #
>   # configuration written to .config
>   #
>   $ make allmodconfig
>   scripts/kconfig/conf  --allmodconfig Kconfig
>   #
>   # No change to .config
>   #
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reported-by: Linus Torvalds <torvalds@linux-foundation.org> ?

:)

Anyway, nice change, looks good to me:


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Masahiro Yamada]

On Fri, May 10, 2019 at 3:21 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 10, 2019 at 03:12:05PM +0900, Masahiro Yamada wrote:
> > Kconfig updates the .config when it exits even if its content is
> > exactly the same as before. Since its timestamp becomes newer than
> > that of other build artifacts, additional processing is invoked,
> > which is annoying.
> >
> > - syncconfig is invoked to update include/config/auto.conf, etc.
> >
> > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
> >   then vmlinux is relinked as well.
> >
> > If the .config is not changed at all, we do not have to even
> > touch it. Just bail out showing "No change to .config".
> >
> >   $ make allmodconfig
> >   scripts/kconfig/conf  --allmodconfig Kconfig
> >   #
> >   # configuration written to .config
> >   #
> >   $ make allmodconfig
> >   scripts/kconfig/conf  --allmodconfig Kconfig
> >   #
> >   # No change to .config
> >   #
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> ?

OK.

For reviewers who do not know the context,
please see this comment from Linus:

https://lkml.org/lkml/2019/5/9/983



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Geert Uytterhoeven]

Hi Yamada-san,

On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Kconfig updates the .config when it exits even if its content is
> exactly the same as before. Since its timestamp becomes newer than
> that of other build artifacts, additional processing is invoked,
> which is annoying.
>
> - syncconfig is invoked to update include/config/auto.conf, etc.
>
> - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
>   then vmlinux is relinked as well.
>
> If the .config is not changed at all, we do not have to even
> touch it. Just bail out showing "No change to .config".
>
>   $ make allmodconfig
>   scripts/kconfig/conf  --allmodconfig Kconfig
>   #
>   # configuration written to .config
>   #
>   $ make allmodconfig
>   scripts/kconfig/conf  --allmodconfig Kconfig
>   #
>   # No change to .config
>   #
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Thanks for your patch!

> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c

> @@ -888,6 +935,13 @@ int conf_write(const char *name)
>         fclose(out);
>
>         if (*tmpname) {
> +               if (is_same(name, tmpname)) {
> +                       conf_message("No change to %s", name);
> +                       unlink(tmpname);
> +                       sym_set_change_count(0);
> +                       return 0;
> +               }
> +
>                 snprintf(oldname, sizeof(oldname), "%s.old", name);
>                 rename(name, oldname);
>                 if (rename(tmpname, name))

This causes a semantic change for the meaning of ".config.old", which is
no longer updated if .config has not changed.
Hence its contents may no longer correspond to the previous config, but to
an arbitrary older version.

My workflow involves always running my own script "linux-oldconfig",
instead of "make oldconfig", so I immediately see what has changed:

    $ cat $(type -p linux-oldconfig)
    #!/bin/bash
    make ${0#*/linux-} && colordiff -u .config{.old,}

However, after your patch, "linux-oldconfig" keeps on showing the old
changes, while .config itself has not changed.  I guess other people
tracking .config changes are using a similar scheme.

One way to fix this is to rename tmpname to oldname if is_same() returns
true. However, this may have side effects for other people, as the
timestamp of .config.old will be changed.

Do you have a better solution?
Thanks!

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Sam Ravnborg]

Hi Geert/Masahiro.

On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:
> Hi Yamada-san,
> 
> On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > Kconfig updates the .config when it exits even if its content is
> > exactly the same as before. Since its timestamp becomes newer than
> > that of other build artifacts, additional processing is invoked,
> > which is annoying.
> >
> > - syncconfig is invoked to update include/config/auto.conf, etc.
> >
> > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
> >   then vmlinux is relinked as well.
> >
> > If the .config is not changed at all, we do not have to even
> > touch it. Just bail out showing "No change to .config".
It would be preferable that if nothing changed no output is generated.
Like we do not tell that we did not build a .o file because the .c file
had not changed.
Less noise for a kernel build where nothings happens.

> This causes a semantic change for the meaning of ".config.old", which is
> no longer updated if .config has not changed.
> Hence its contents may no longer correspond to the previous config, but to
> an arbitrary older version.
This semantic change is good.
So we now have a .config.old that correspond to the state before
the last change. Also after several kernel builds.

> > My workflow involves always running my own script "linux-oldconfig",
> instead of "make oldconfig", so I immediately see what has changed:
> 
>     $ cat $(type -p linux-oldconfig)
>     #!/bin/bash
>     make ${0#*/linux-} && colordiff -u .config{.old,}
So scripts relying on the old (broken) behaviour will no longer work.
The new behaviour is better as it is usefaul in many typical situations.

Hacking, hack. What did I change in the config?

	Sam

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Geert Uytterhoeven]

Hi Sam,

On Fri, May 10, 2019 at 9:03 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:
> > On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > > Kconfig updates the .config when it exits even if its content is
> > > exactly the same as before. Since its timestamp becomes newer than
> > > that of other build artifacts, additional processing is invoked,
> > > which is annoying.
> > >
> > > - syncconfig is invoked to update include/config/auto.conf, etc.
> > >
> > > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
> > >   then vmlinux is relinked as well.
> > >
> > > If the .config is not changed at all, we do not have to even
> > > touch it. Just bail out showing "No change to .config".

> > This causes a semantic change for the meaning of ".config.old", which is
> > no longer updated if .config has not changed.
> > Hence its contents may no longer correspond to the previous config, but to
> > an arbitrary older version.
> This semantic change is good.
> So we now have a .config.old that correspond to the state before
> the last change. Also after several kernel builds.
>
> > > My workflow involves always running my own script "linux-oldconfig",
> > instead of "make oldconfig", so I immediately see what has changed:
> >
> >     $ cat $(type -p linux-oldconfig)
> >     #!/bin/bash
> >     make ${0#*/linux-} && colordiff -u .config{.old,}
> So scripts relying on the old (broken) behaviour will no longer work.
> The new behaviour is better as it is usefaul in many typical situations.
>
> Hacking, hack. What did I change in the config?

I agree both semantics have their merits.
Sometimes I also wanted to see the last real change...

No worries, updating my script, so it works with both semantics:

    $ cat $(type -p linux-oldconfig)
    #!/bin/bash
    cp -a .config .config.orig
    make ${0#*/linux-} && colordiff -u .config{.orig,}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] kconfig: do not write .config if the content is the same

[Masahiro Yamada]

Hi Sam, Geert,

On Fri, May 10, 2019 at 4:04 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Geert/Masahiro.
>
> On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:
> > Hi Yamada-san,
> >
> > On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > > Kconfig updates the .config when it exits even if its content is
> > > exactly the same as before. Since its timestamp becomes newer than
> > > that of other build artifacts, additional processing is invoked,
> > > which is annoying.
> > >
> > > - syncconfig is invoked to update include/config/auto.conf, etc.
> > >
> > > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
> > >   then vmlinux is relinked as well.
> > >
> > > If the .config is not changed at all, we do not have to even
> > > touch it. Just bail out showing "No change to .config".
> It would be preferable that if nothing changed no output is generated.
> Like we do not tell that we did not build a .o file because the .c file
> had not changed.
> Less noise for a kernel build where nothings happens.
>
> > This causes a semantic change for the meaning of ".config.old", which is
> > no longer updated if .config has not changed.
> > Hence its contents may no longer correspond to the previous config, but to
> > an arbitrary older version.
> This semantic change is good.
> So we now have a .config.old that correspond to the state before
> the last change. Also after several kernel builds.


I agree.

When there is no change in the configuration,
Kconfig will not even attempt to output anything.

Updating only .config.old is strange.

Thanks.



--
Best Regards
Masahiro Yamada