2017-07-17 17:14 GMT+02:00 Ian Jackson <ian.jack...@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.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
