Re: [Xen-devel] [PATCH v2] xenconsole: Add pipe option

[Felix Schmoll <eggi.innovations@xxxxxxxxx>]

2017-07-17 17:14 GMT+02:00 Ian Jackson <ian.jackson@xxxxxxxxxxxxx>:

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel