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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@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?

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

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