[PATCH 1/2] kconfig: remove check_stdin()

[PATCH 1/2] kconfig: remove check_stdin()

[Masahiro Yamada]

Except silentoldconfig, valid_stdin is 1, so check_stdin() is no-op.

oldconfig and silentoldconfig work almost in the same way except that
the latter generates additional files.  Both ask users for input for
new symbols.

I do not know why only silentoldconfig requires stdio be tty.

  $ rm -f .config; touch .config
  $ yes "" | make oldconfig > stdout
  $ rm -f .config; touch .config
  $ yes "" | make silentoldconfig > stdout
  make[1]: *** [silentoldconfig] Error 1
  make: *** [silentoldconfig] Error 2
  $ tail -n 4 stdout
  Console input/output is redirected. Run 'make oldconfig' to update configuration.

  scripts/kconfig/Makefile:40: recipe for target 'silentoldconfig' failed
  Makefile:507: recipe for target 'silentoldconfig' failed

Redirection is useful, for example, for testing where we want to give
particular key inputs from a test file, then check the result.

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

 scripts/kconfig/conf.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 307bc3f..358e2e4 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -39,7 +39,6 @@ static enum input_mode input_mode = oldaskconfig;
 
 static int indent = 1;
 static int tty_stdio;
-static int valid_stdin = 1;
 static int sync_kconfig;
 static int conf_cnt;
 static char line[PATH_MAX];
@@ -72,16 +71,6 @@ static void strip(char *str)
 		*p-- = 0;
 }
 
-static void check_stdin(void)
-{
-	if (!valid_stdin) {
-		printf(_("aborted!\n\n"));
-		printf(_("Console input/output is redirected. "));
-		printf(_("Run 'make oldconfig' to update configuration.\n\n"));
-		exit(1);
-	}
-}
-
 /* Helper function to facilitate fgets() by Jean Sacren. */
 static void xfgets(char *str, int size, FILE *in)
 {
@@ -113,7 +102,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
 			printf("%s\n", def);
 			return 0;
 		}
-		check_stdin();
 		/* fall through */
 	case oldaskconfig:
 		fflush(stdout);
@@ -315,7 +303,6 @@ static int conf_choice(struct menu *menu)
 				printf("%d\n", cnt);
 				break;
 			}
-			check_stdin();
 			/* fall through */
 		case oldaskconfig:
 			fflush(stdout);
@@ -650,7 +637,6 @@ int main(int ac, char **av)
 				return 1;
 			}
 		}
-		valid_stdin = tty_stdio;
 	}
 
 	switch (input_mode) {
-- 
2.7.4

[PATCH 2/2] kconfig: echo stdin to stdout if either is redirected

[Masahiro Yamada]

If stdio is not tty, conf_askvalue() puts additional new line to
prevent prompts are all concatenated into a single line.  This care
is missing in conf_choice(), so a 'choice' prompt and the next prompt
are shown in the same line.

Move the code into xfgets() to take care of all cases.  To improve
this more, echo stdin to stdout.  This clarifies what keys were input
from stdio and the stdout looks like as if it were from tty.

I removed the isatty(2) check since stderr is unrelated here.

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

 scripts/kconfig/conf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 358e2e4..c5318d3 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -76,6 +76,9 @@ static void xfgets(char *str, int size, FILE *in)
 {
 	if (!fgets(str, size, in))
 		fprintf(stderr, "\nError in reading or end of file.\n");
+
+	if (!tty_stdio)
+		printf("%s", str);
 }
 
 static int conf_askvalue(struct symbol *sym, const char *def)
@@ -106,8 +109,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
 	case oldaskconfig:
 		fflush(stdout);
 		xfgets(line, sizeof(line), stdin);
-		if (!tty_stdio)
-			printf("\n");
 		return 1;
 	default:
 		break;
@@ -495,7 +496,7 @@ int main(int ac, char **av)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
-	tty_stdio = isatty(0) && isatty(1) && isatty(2);
+	tty_stdio = isatty(0) && isatty(1);
 
 	while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) {
 		if (opt == 's') {
-- 
2.7.4

Re: [PATCH 2/2] kconfig: echo stdin to stdout if either is redirected

[Ulf Magnusson]

On Thu, Feb 8, 2018 at 6:56 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> If stdio is not tty, conf_askvalue() puts additional new line to
> prevent prompts are all concatenated into a single line.  This care
> is missing in conf_choice(), so a 'choice' prompt and the next prompt
> are shown in the same line.
>
> Move the code into xfgets() to take care of all cases.  To improve
> this more, echo stdin to stdout.  This clarifies what keys were input
> from stdio and the stdout looks like as if it were from tty.
>
> I removed the isatty(2) check since stderr is unrelated here.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/kconfig/conf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 358e2e4..c5318d3 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -76,6 +76,9 @@ static void xfgets(char *str, int size, FILE *in)
>  {
>         if (!fgets(str, size, in))
>                 fprintf(stderr, "\nError in reading or end of file.\n");
> +
> +       if (!tty_stdio)
> +               printf("%s", str);
>  }
>
>  static int conf_askvalue(struct symbol *sym, const char *def)
> @@ -106,8 +109,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>         case oldaskconfig:
>                 fflush(stdout);
>                 xfgets(line, sizeof(line), stdin);
> -               if (!tty_stdio)
> -                       printf("\n");
>                 return 1;
>         default:
>                 break;
> @@ -495,7 +496,7 @@ int main(int ac, char **av)
>         bindtextdomain(PACKAGE, LOCALEDIR);
>         textdomain(PACKAGE);
>
> -       tty_stdio = isatty(0) && isatty(1) && isatty(2);
> +       tty_stdio = isatty(0) && isatty(1);
>
>         while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) {
>                 if (opt == 's') {
> --
> 2.7.4
>

Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>

Might be some case I'm not thinking of, but wouldn't it make sense to
just check isatty(1) as well? If stdout is a regular file, it seems
it'd be nice to have the input appear there, regardless of where stdin
is from.

Maybe the tty_stdio variable could be removed then as well, replaced
with just 'if (!isatty(1))'.

Cheers,
Ulf

Re: [PATCH 2/2] kconfig: echo stdin to stdout if either is redirected

[Ulf Magnusson]

On Thu, Feb 8, 2018 at 7:35 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote:
> On Thu, Feb 8, 2018 at 6:56 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> If stdio is not tty, conf_askvalue() puts additional new line to
>> prevent prompts are all concatenated into a single line.  This care
>> is missing in conf_choice(), so a 'choice' prompt and the next prompt
>> are shown in the same line.
>>
>> Move the code into xfgets() to take care of all cases.  To improve
>> this more, echo stdin to stdout.  This clarifies what keys were input
>> from stdio and the stdout looks like as if it were from tty.
>>
>> I removed the isatty(2) check since stderr is unrelated here.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  scripts/kconfig/conf.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>> index 358e2e4..c5318d3 100644
>> --- a/scripts/kconfig/conf.c
>> +++ b/scripts/kconfig/conf.c
>> @@ -76,6 +76,9 @@ static void xfgets(char *str, int size, FILE *in)
>>  {
>>         if (!fgets(str, size, in))
>>                 fprintf(stderr, "\nError in reading or end of file.\n");
>> +
>> +       if (!tty_stdio)
>> +               printf("%s", str);
>>  }
>>
>>  static int conf_askvalue(struct symbol *sym, const char *def)
>> @@ -106,8 +109,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>>         case oldaskconfig:
>>                 fflush(stdout);
>>                 xfgets(line, sizeof(line), stdin);
>> -               if (!tty_stdio)
>> -                       printf("\n");
>>                 return 1;
>>         default:
>>                 break;
>> @@ -495,7 +496,7 @@ int main(int ac, char **av)
>>         bindtextdomain(PACKAGE, LOCALEDIR);
>>         textdomain(PACKAGE);
>>
>> -       tty_stdio = isatty(0) && isatty(1) && isatty(2);
>> +       tty_stdio = isatty(0) && isatty(1);
>>
>>         while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) {
>>                 if (opt == 's') {
>> --
>> 2.7.4
>>
>
> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>
>
> Might be some case I'm not thinking of, but wouldn't it make sense to
> just check isatty(1) as well? If stdout is a regular file, it seems
> it'd be nice to have the input appear there, regardless of where stdin
> is from.
>
> Maybe the tty_stdio variable could be removed then as well, replaced
> with just 'if (!isatty(1))'.
>
> Cheers,
> Ulf

Hmm... if stdout is a tty and stdin isn't, this would prevent the
input from showing up on the terminal though, so I guess it makes
sense. That case seems more important than the weird
stdin=tty/stdout=file case.

Tricky stuff. :)

Cheers,
Ulf

Re: [PATCH 2/2] kconfig: echo stdin to stdout if either is redirected

[Masahiro Yamada]

2018-02-08 15:51 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> On Thu, Feb 8, 2018 at 7:35 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote:
>> On Thu, Feb 8, 2018 at 6:56 AM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>> If stdio is not tty, conf_askvalue() puts additional new line to
>>> prevent prompts are all concatenated into a single line.  This care
>>> is missing in conf_choice(), so a 'choice' prompt and the next prompt
>>> are shown in the same line.
>>>
>>> Move the code into xfgets() to take care of all cases.  To improve
>>> this more, echo stdin to stdout.  This clarifies what keys were input
>>> from stdio and the stdout looks like as if it were from tty.
>>>
>>> I removed the isatty(2) check since stderr is unrelated here.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  scripts/kconfig/conf.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>>> index 358e2e4..c5318d3 100644
>>> --- a/scripts/kconfig/conf.c
>>> +++ b/scripts/kconfig/conf.c
>>> @@ -76,6 +76,9 @@ static void xfgets(char *str, int size, FILE *in)
>>>  {
>>>         if (!fgets(str, size, in))
>>>                 fprintf(stderr, "\nError in reading or end of file.\n");
>>> +
>>> +       if (!tty_stdio)
>>> +               printf("%s", str);
>>>  }
>>>
>>>  static int conf_askvalue(struct symbol *sym, const char *def)
>>> @@ -106,8 +109,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>>>         case oldaskconfig:
>>>                 fflush(stdout);
>>>                 xfgets(line, sizeof(line), stdin);
>>> -               if (!tty_stdio)
>>> -                       printf("\n");
>>>                 return 1;
>>>         default:
>>>                 break;
>>> @@ -495,7 +496,7 @@ int main(int ac, char **av)
>>>         bindtextdomain(PACKAGE, LOCALEDIR);
>>>         textdomain(PACKAGE);
>>>
>>> -       tty_stdio = isatty(0) && isatty(1) && isatty(2);
>>> +       tty_stdio = isatty(0) && isatty(1);
>>>
>>>         while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) {
>>>                 if (opt == 's') {
>>> --
>>> 2.7.4
>>>
>>
>> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>
>>
>> Might be some case I'm not thinking of, but wouldn't it make sense to
>> just check isatty(1) as well? If stdout is a regular file, it seems
>> it'd be nice to have the input appear there, regardless of where stdin
>> is from.
>>
>> Maybe the tty_stdio variable could be removed then as well, replaced
>> with just 'if (!isatty(1))'.
>>
>> Cheers,
>> Ulf
>
> Hmm... if stdout is a tty and stdin isn't, this would prevent the
> input from showing up on the terminal though, so I guess it makes
> sense.

Yes.  I want to address this case too.

Anyway, thank you for checking the details!




> That case seems more important than the weird
> stdin=tty/stdout=file case.
>
> Tricky stuff. :)
>
> Cheers,
> Ulf
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kconfig: echo stdin to stdout if either is redirected

[Ulf Magnusson]

On Thu, Feb 8, 2018 at 7:51 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote:
> On Thu, Feb 8, 2018 at 7:35 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote:
>> On Thu, Feb 8, 2018 at 6:56 AM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>> If stdio is not tty, conf_askvalue() puts additional new line to
>>> prevent prompts are all concatenated into a single line.  This care
>>> is missing in conf_choice(), so a 'choice' prompt and the next prompt
>>> are shown in the same line.
>>>
>>> Move the code into xfgets() to take care of all cases.  To improve
>>> this more, echo stdin to stdout.  This clarifies what keys were input
>>> from stdio and the stdout looks like as if it were from tty.
>>>
>>> I removed the isatty(2) check since stderr is unrelated here.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  scripts/kconfig/conf.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>>> index 358e2e4..c5318d3 100644
>>> --- a/scripts/kconfig/conf.c
>>> +++ b/scripts/kconfig/conf.c
>>> @@ -76,6 +76,9 @@ static void xfgets(char *str, int size, FILE *in)
>>>  {
>>>         if (!fgets(str, size, in))
>>>                 fprintf(stderr, "\nError in reading or end of file.\n");
>>> +
>>> +       if (!tty_stdio)
>>> +               printf("%s", str);
>>>  }
>>>
>>>  static int conf_askvalue(struct symbol *sym, const char *def)
>>> @@ -106,8 +109,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>>>         case oldaskconfig:
>>>                 fflush(stdout);
>>>                 xfgets(line, sizeof(line), stdin);
>>> -               if (!tty_stdio)
>>> -                       printf("\n");
>>>                 return 1;
>>>         default:
>>>                 break;
>>> @@ -495,7 +496,7 @@ int main(int ac, char **av)
>>>         bindtextdomain(PACKAGE, LOCALEDIR);
>>>         textdomain(PACKAGE);
>>>
>>> -       tty_stdio = isatty(0) && isatty(1) && isatty(2);
>>> +       tty_stdio = isatty(0) && isatty(1);
>>>
>>>         while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) {
>>>                 if (opt == 's') {
>>> --
>>> 2.7.4
>>>
>>
>> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>
>>
>> Might be some case I'm not thinking of, but wouldn't it make sense to
>> just check isatty(1) as well? If stdout is a regular file, it seems
>> it'd be nice to have the input appear there, regardless of where stdin
>> is from.
>>
>> Maybe the tty_stdio variable could be removed then as well, replaced
>> with just 'if (!isatty(1))'.
>>
>> Cheers,
>> Ulf
>
> Hmm... if stdout is a tty and stdin isn't, this would prevent the
> input from showing up on the terminal though, so I guess it makes
> sense. That case seems more important than the weird
> stdin=tty/stdout=file case.
>
> Tricky stuff. :)
>
> Cheers,
> Ulf

Oh... and that one's already taken care of too, reading closer.

stdin | stdout | wants stdin written to stdout
------+--------+-------------------------------------
tty   | tty    | no (already echoed by tty)
tty   | file   | yes (echoed by tty, written to file)
file  | tty    | yes (not echoed by tty)
file  | file   | yes

So !(tty(0) && tty(1)) makes sense. Excuse the rambling...

Cheers,
Ulf

Re: [PATCH 1/2] kconfig: remove check_stdin()

[Ulf Magnusson]

On Thu, Feb 8, 2018 at 6:56 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Except silentoldconfig, valid_stdin is 1, so check_stdin() is no-op.
>
> oldconfig and silentoldconfig work almost in the same way except that
> the latter generates additional files.  Both ask users for input for
> new symbols.
>
> I do not know why only silentoldconfig requires stdio be tty.
>
>   $ rm -f .config; touch .config
>   $ yes "" | make oldconfig > stdout
>   $ rm -f .config; touch .config
>   $ yes "" | make silentoldconfig > stdout
>   make[1]: *** [silentoldconfig] Error 1
>   make: *** [silentoldconfig] Error 2
>   $ tail -n 4 stdout
>   Console input/output is redirected. Run 'make oldconfig' to update configuration.
>
>   scripts/kconfig/Makefile:40: recipe for target 'silentoldconfig' failed
>   Makefile:507: recipe for target 'silentoldconfig' failed
>
> Redirection is useful, for example, for testing where we want to give
> particular key inputs from a test file, then check the result.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/kconfig/conf.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 307bc3f..358e2e4 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -39,7 +39,6 @@ static enum input_mode input_mode = oldaskconfig;
>
>  static int indent = 1;
>  static int tty_stdio;
> -static int valid_stdin = 1;
>  static int sync_kconfig;
>  static int conf_cnt;
>  static char line[PATH_MAX];
> @@ -72,16 +71,6 @@ static void strip(char *str)
>                 *p-- = 0;
>  }
>
> -static void check_stdin(void)
> -{
> -       if (!valid_stdin) {
> -               printf(_("aborted!\n\n"));
> -               printf(_("Console input/output is redirected. "));
> -               printf(_("Run 'make oldconfig' to update configuration.\n\n"));
> -               exit(1);
> -       }
> -}
> -
>  /* Helper function to facilitate fgets() by Jean Sacren. */
>  static void xfgets(char *str, int size, FILE *in)
>  {
> @@ -113,7 +102,6 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>                         printf("%s\n", def);
>                         return 0;
>                 }
> -               check_stdin();
>                 /* fall through */
>         case oldaskconfig:
>                 fflush(stdout);
> @@ -315,7 +303,6 @@ static int conf_choice(struct menu *menu)
>                                 printf("%d\n", cnt);
>                                 break;
>                         }
> -                       check_stdin();
>                         /* fall through */
>                 case oldaskconfig:
>                         fflush(stdout);
> @@ -650,7 +637,6 @@ int main(int ac, char **av)
>                                 return 1;
>                         }
>                 }
> -               valid_stdin = tty_stdio;
>         }
>
>         switch (input_mode) {
> --
> 2.7.4
>

Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com>

Lots of weird stuff indeed...

Cheers,
Ulf

Re: [PATCH 1/2] kconfig: remove check_stdin()

[Sam Ravnborg]

On Thu, Feb 08, 2018 at 02:56:39PM +0900, Masahiro Yamada wrote:
> Except silentoldconfig, valid_stdin is 1, so check_stdin() is no-op.
> 
> oldconfig and silentoldconfig work almost in the same way except that
> the latter generates additional files.  Both ask users for input for
> new symbols.
> 
> I do not know why only silentoldconfig requires stdio be tty.

The general idea was to error out if stdout was not a tty and
kconfig wanted to prompt the user for anything.

So we avoided having a kconfig that would hang waiting for
user inputs when the user could not see that anything was prompted for.

The actual implementation may not follow this today as many seems not
to be aware of this little trick.

	Sam