diff mbox

[v2] xenconsole: Add pipe option

Message ID 20170717094911.3966-1-eggi.innovations@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Felix Schmoll July 17, 2017, 9:49 a.m. UTC
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(-)

Comments

Ian Jackson July 17, 2017, 3:14 p.m. UTC | #1
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.
Felix Schmoll July 19, 2017, 9:09 a.m. UTC | #2
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.
>
Wei Liu July 19, 2017, 10:41 a.m. UTC | #3
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
Ian Jackson July 19, 2017, 1:10 p.m. UTC | #4
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 mbox

Patch

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);