Re: [PATCH] xenconsole: Allow non-interactive use

[Martin Lucina <martin@lucina.net>]

On Thursday, 23.07.2015 at 09:48, Ian Campbell wrote:
> On Wed, 2015-07-22 at 19:08 +0200, Martin Lucina wrote:
> > If xenconsole is run with stdin closed or redirected to /dev/null,
> > console_loop() will return immediately due to failure to read from
> > STDIN_FILENO. This patch tests if stdin and stdout are both connected 
> > to
> > a TTY and, if not, xenconsole will not attempt to read from stdin or
> > modify stdout terminal attributes.
> > 
> > Existing behaviour when xenconsole is run from a terminal does not
> > change.
> > 
> > This allows for non-interactive use, eg. running "xl create -c" under
> > systemd or piping the output of "xl console" to another command.
> > 
> > Signed-off-by: Martin Lucina <martin@lucina.net>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> WRT the 4.6 freeze I'm torn between calling this a feature or a bugfix.
> 
> A pair of nits, which probably aren't worth acting on:
> 
> > @@ -176,8 +177,13 @@ static int console_loop(int fd, struct xs_handle
> > *xs, char *pty_path)
> >  		fd_set fds;
> >  
> >  		FD_ZERO(&fds);
> > -		FD_SET(STDIN_FILENO, &fds);
> > -		max_fd = STDIN_FILENO;
> > +		if (interactive) {
> > +			FD_SET(STDIN_FILENO, &fds);
> > +			max_fd = STDIN_FILENO;
> > +		}
> > +		else {
> > +			max_fd = -1;
> > +		}
> 
> Looking at the rest of the file and tools/console subtree it seems the
> prevailing coding style is:
> 
> 		} else
>                 	                max_fd = -1;
> 
> (i.e. } brace on the same line as the else and no {} for single
> statements after an else).
> 
> But maybe it would be better to set max_fd = -1 on declaration and do
> the max dance here as with the following cases?

Declaring max_fd = -1 is indeed clearer, I can do a v2 with that change if
you like.

One other bug that my change makes potentially easier to trigger is that
you can run "xl console DOMID" multiple times with the same DOMID and the
result is broken; each instance gets part of the data written to the
console.

Any ideas on how to address this in a simple fashion?

Martin