| Message ID | 20170717094911.3966-1-eggi.innovations@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Felix Schmoll writes ("[PATCH v2] xenconsole: Add pipe option"):
> Add pipe option to xenconsole that forwards console input.
Thanks. IMO the commit message could do with better explanation. It
should mention that xenconsole has a strange behaviour where it
doesn't forward stdin unless stdin and stdout are both ttys, and your
option is to disable this.
Also "interactive" (used in the code) is a bit of a funny name for
this, but "pipe" is worse IMO. It would work fine for a socket (eg
from inetd), for example. How about calling the option
"--interactive" or "--bidirectional" or something ?
The code LGTM.
Thanks,
Ian.
2017-07-17 17:14 GMT+02:00 Ian Jackson <ian.jackson@eu.citrix.com>: > Felix Schmoll writes ("[PATCH v2] xenconsole: Add pipe option"): > > Add pipe option to xenconsole that forwards console input. > > Thanks. IMO the commit message could do with better explanation. It > should mention that xenconsole has a strange behaviour where it > doesn't forward stdin unless stdin and stdout are both ttys, and your > option is to disable this. > > Also "interactive" (used in the code) is a bit of a funny name for > this, but "pipe" is worse IMO. It would work fine for a socket (eg > from inetd), for example. How about calling the option > "--interactive" or "--bidirectional" or something ? > > As there is already an interactive variable in the code, it seems like a rather strange overloading to call the option interactive that directly affects a different variable (currently pipe). The name seems to make sense however, so I propose to simplify the code by removing the isatty-check from line 349 and moving it to line 472, resulting in the following: 472 if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO)) { 473 interactive = 1; 474 init_term(STDIN_FILENO, &stdin_old_attr); 475 atexit(restore_term_stdin); /* if this fails, oh dear */ 476 } Then the interactive-variable is free for my purposes, so there is no need to introduce a new variable at all. Or is there anything that requires the check to be at the top? As the new commit message I suggest: Add option to xenconsole to always forward console input Currently the default behaviour of the xenconsole client is to ignore any input to stdin, unless stdin and stdout are both ttys. The new option allows to manually overwrite this, causing the client to forward input regardless. > The code LGTM. > > Thanks, > Ian. >
On Wed, Jul 19, 2017 at 11:09:57AM +0200, Felix Schmoll wrote: > 2017-07-17 17:14 GMT+02:00 Ian Jackson <ian.jackson@eu.citrix.com>: > > > Felix Schmoll writes ("[PATCH v2] xenconsole: Add pipe option"): > > > Add pipe option to xenconsole that forwards console input. > > > > Thanks. IMO the commit message could do with better explanation. It > > should mention that xenconsole has a strange behaviour where it > > doesn't forward stdin unless stdin and stdout are both ttys, and your > > option is to disable this. > > > > Also "interactive" (used in the code) is a bit of a funny name for > > this, but "pipe" is worse IMO. It would work fine for a socket (eg > > from inetd), for example. How about calling the option > > "--interactive" or "--bidirectional" or something ? > > > > > As there is already an interactive variable in the code, it seems like a > rather strange overloading to call the option interactive that directly > affects a different variable (currently pipe). The name seems to make sense > however, so I propose to simplify the code by removing the isatty-check > from line 349 and moving it to line 472, resulting in the following: > > 472 if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO)) { > 473 interactive = 1; > 474 init_term(STDIN_FILENO, &stdin_old_attr); > 475 atexit(restore_term_stdin); /* if this fails, oh dear */ > > 476 } > > Then the interactive-variable is free for my purposes, so there is no need > to introduce a new variable at all. > > Or is there anything that requires the check to be at the top? It doesn't matter as long as it is done before entering the main loop. I don't think the internal variable name is hugely important. Just change the option name to --interactive would be fine by me. > > > As the new commit message I suggest: > > Add option to xenconsole to always forward console input > > Currently the default behaviour of the xenconsole client is to ignore any > input to stdin, unless stdin and stdout are both ttys. The new option > allows to manually overwrite this, causing the client to forward input > regardless. LGTM
Felix Schmoll writes ("Re: [PATCH v2] xenconsole: Add pipe option"):
> As there is already an interactive variable in the code, it seems
> like a rather strange overloading to call the option interactive
> that directly affects a different variable (currently pipe). The
> name seems to make sense however,
Right, I think the UI should be driven by the needs of the human
who'll use it, not by the variable names in the code.
> so I propose to simplify the code
> by removing the isatty-check from line 349 and moving it to line
> 472, resulting in the following:
>
> 472 if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO)) {
> 473 interactive = 1;
> 474 init_term(STDIN_FILENO, &stdin_old_attr);
> 475 atexit(restore_term_stdin); /* if this fails, oh dear */
> 476 }
>
> Then the interactive-variable is free for my purposes, so there is no need to
> introduce a new variable at all.
>
> Or is there anything that requires the check to be at the top?
I doubt it. Doing it after the option parsing loop would be much more
conventional.
> As the new commit message I suggest:
>
> Add option to xenconsole to always forward console input
>
> Currently the default behaviour of the xenconsole client is to
> ignore any input to stdin, unless stdin and stdout are both
> ttys. The new option allows to manually overwrite this, causing the
> client to forward input regardless.
SGTM.
Thanks,
Ian.
diff --git a/tools/console/client/main.c b/tools/console/client/main.c index 977779f034..84a466c32f 100644 --- a/tools/console/client/main.c +++ b/tools/console/client/main.c @@ -334,6 +334,7 @@ int main(int argc, char **argv) { "num", 1, 0, 'n' }, { "help", 0, 0, 'h' }, { "start-notify-fd", 1, 0, 's' }, + { "pipe", 0, 0, 'p' }, { 0 }, }; @@ -343,6 +344,7 @@ int main(int argc, char **argv) char *end; console_type type = CONSOLE_INVAL; bool interactive = 0; + bool pipe = 0; if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO)) interactive = 1; @@ -370,6 +372,9 @@ int main(int argc, char **argv) case 's': start_notify_fd = atoi(optarg); break; + case 'p': + pipe = 1; + break; default: fprintf(stderr, "Invalid argument\n"); fprintf(stderr, "Try `%s --help' for more information.\n", @@ -484,7 +489,7 @@ int main(int argc, char **argv) close(start_notify_fd); } - console_loop(spty, xs, path, interactive); + console_loop(spty, xs, path, interactive || pipe); free(path); free(dom_path);
Add pipe option to xenconsole that forwards console input. Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com> --- Changed since v1: * introduce separate pipe flag * remove changes to libxl --- tools/console/client/main.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)