| Message ID | 20171208133416.28978-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code | expand |
On 12/08/2017 07:34 AM, Daniel P. Berrange wrote: > qemu-io puts the TTY into non-canonical mode, which means no EOF processing is > done and thus getchar() will never return the EOF constant. Instead we have to > query the TTY attributes to determine the configured EOF character (usually > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the > regression that prevented Ctrl-D from triggering an exit of qemu-io that has > existed since readline was first added in > > commit 0cf17e181798063c3824c8200ba46f25f54faa1a > Author: Stefan Hajnoczi <stefanha@redhat.com> > Date: Thu Nov 14 11:54:17 2013 +0100 > > qemu-io: use readline.c > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > Changed in v2: > +++ b/qemu-io.c > @@ -10,6 +10,9 @@ > #include "qemu/osdep.h" > #include <getopt.h> > #include <libgen.h> > +#ifndef _WIN32 > +#include <termios.h> > +#endif Wouldn't a configure probe for the existence of <termios.h> be more reliable than just hard-coding the list of platforms where it is currently not found? > > #include "qapi/error.h" > #include "qemu-io.h" > @@ -41,6 +44,26 @@ static bool imageOpts; > > static ReadLineState *readline_state; > > +static int ttyEOF; > + > +static int get_eof_char(void) > +{ > +#ifdef _WIN32 in which case this also should be #if HAVE_TERMIOS_H But I guess unless someone complains, all other platforms that we care about have termios.h, so a weak: Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri, Dec 08, 2017 at 10:21:35AM -0600, Eric Blake wrote: > On 12/08/2017 07:34 AM, Daniel P. Berrange wrote: > > qemu-io puts the TTY into non-canonical mode, which means no EOF processing is > > done and thus getchar() will never return the EOF constant. Instead we have to > > query the TTY attributes to determine the configured EOF character (usually > > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the > > regression that prevented Ctrl-D from triggering an exit of qemu-io that has > > existed since readline was first added in > > > > commit 0cf17e181798063c3824c8200ba46f25f54faa1a > > Author: Stefan Hajnoczi <stefanha@redhat.com> > > Date: Thu Nov 14 11:54:17 2013 +0100 > > > > qemu-io: use readline.c > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > Changed in v2: > > > +++ b/qemu-io.c > > @@ -10,6 +10,9 @@ > > #include "qemu/osdep.h" > > #include <getopt.h> > > #include <libgen.h> > > +#ifndef _WIN32 > > +#include <termios.h> > > +#endif > > Wouldn't a configure probe for the existence of <termios.h> be more > reliable than just hard-coding the list of platforms where it is > currently not found? > > > > > #include "qapi/error.h" > > #include "qemu-io.h" > > @@ -41,6 +44,26 @@ static bool imageOpts; > > > > static ReadLineState *readline_state; > > > > +static int ttyEOF; > > + > > +static int get_eof_char(void) > > +{ > > +#ifdef _WIN32 > > in which case this also should be #if HAVE_TERMIOS_H > > But I guess unless someone complains, all other platforms that we care > about have termios.h, so a weak: FYI, vl.c already includes termios.h, with merely #ifndef _WIN32 protection, so that shows this is sufficient for our immediate needs. > Reviewed-by: Eric Blake <eblake@redhat.com> Regards, Daniel
ping, does any block maintainer want to queue this one ? On Fri, Dec 08, 2017 at 01:34:16PM +0000, Daniel P. Berrange wrote: > qemu-io puts the TTY into non-canonical mode, which means no EOF processing is > done and thus getchar() will never return the EOF constant. Instead we have to > query the TTY attributes to determine the configured EOF character (usually > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the > regression that prevented Ctrl-D from triggering an exit of qemu-io that has > existed since readline was first added in > > commit 0cf17e181798063c3824c8200ba46f25f54faa1a > Author: Stefan Hajnoczi <stefanha@redhat.com> > Date: Thu Nov 14 11:54:17 2013 +0100 > > qemu-io: use readline.c > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > Changed in v2: > > - Query termios settings for EOF character > > qemu-io.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/qemu-io.c b/qemu-io.c > index c70bde3eb1..fa4972d734 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -10,6 +10,9 @@ > #include "qemu/osdep.h" > #include <getopt.h> > #include <libgen.h> > +#ifndef _WIN32 > +#include <termios.h> > +#endif > > #include "qapi/error.h" > #include "qemu-io.h" > @@ -41,6 +44,26 @@ static bool imageOpts; > > static ReadLineState *readline_state; > > +static int ttyEOF; > + > +static int get_eof_char(void) > +{ > +#ifdef _WIN32 > + return 0x4; /* Ctrl-D */ > +#else > + struct termios tty; > + if (tcgetattr(STDIN_FILENO, &tty) != 0) { > + if (errno == ENOTTY) { > + return 0x0; /* just expect read() == 0 */ > + } else { > + return 0x4; /* Ctrl-D */ > + } > + } > + > + return tty.c_cc[VEOF]; > +#endif > +} > + > static int close_f(BlockBackend *blk, int argc, char **argv) > { > blk_unref(qemuio_blk); > @@ -322,7 +345,7 @@ static char *fetchline_readline(void) > readline_start(readline_state, get_prompt(), 0, readline_func, &line); > while (!line) { > int ch = getchar(); > - if (ch == EOF) { > + if (ttyEOF != 0x0 && ch == ttyEOF) { > break; > } > readline_handle_byte(readline_state, ch); > @@ -592,6 +615,7 @@ int main(int argc, char **argv) > qemuio_add_command(&close_cmd); > > if (isatty(STDIN_FILENO)) { > + ttyEOF = get_eof_char(); > readline_state = readline_init(readline_printf_func, > readline_flush_func, > NULL, > -- > 2.14.3 > Regards, Daniel
Re-ping. On Thu, Jan 25, 2018 at 05:05:01PM +0000, Daniel P. Berrangé wrote: > ping, does any block maintainer want to queue this one ? > > On Fri, Dec 08, 2017 at 01:34:16PM +0000, Daniel P. Berrange wrote: > > qemu-io puts the TTY into non-canonical mode, which means no EOF processing is > > done and thus getchar() will never return the EOF constant. Instead we have to > > query the TTY attributes to determine the configured EOF character (usually > > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the > > regression that prevented Ctrl-D from triggering an exit of qemu-io that has > > existed since readline was first added in > > > > commit 0cf17e181798063c3824c8200ba46f25f54faa1a > > Author: Stefan Hajnoczi <stefanha@redhat.com> > > Date: Thu Nov 14 11:54:17 2013 +0100 > > > > qemu-io: use readline.c > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > Changed in v2: > > > > - Query termios settings for EOF character > > > > qemu-io.c | 26 +++++++++++++++++++++++++- > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/qemu-io.c b/qemu-io.c > > index c70bde3eb1..fa4972d734 100644 > > --- a/qemu-io.c > > +++ b/qemu-io.c > > @@ -10,6 +10,9 @@ > > #include "qemu/osdep.h" > > #include <getopt.h> > > #include <libgen.h> > > +#ifndef _WIN32 > > +#include <termios.h> > > +#endif > > > > #include "qapi/error.h" > > #include "qemu-io.h" > > @@ -41,6 +44,26 @@ static bool imageOpts; > > > > static ReadLineState *readline_state; > > > > +static int ttyEOF; > > + > > +static int get_eof_char(void) > > +{ > > +#ifdef _WIN32 > > + return 0x4; /* Ctrl-D */ > > +#else > > + struct termios tty; > > + if (tcgetattr(STDIN_FILENO, &tty) != 0) { > > + if (errno == ENOTTY) { > > + return 0x0; /* just expect read() == 0 */ > > + } else { > > + return 0x4; /* Ctrl-D */ > > + } > > + } > > + > > + return tty.c_cc[VEOF]; > > +#endif > > +} > > + > > static int close_f(BlockBackend *blk, int argc, char **argv) > > { > > blk_unref(qemuio_blk); > > @@ -322,7 +345,7 @@ static char *fetchline_readline(void) > > readline_start(readline_state, get_prompt(), 0, readline_func, &line); > > while (!line) { > > int ch = getchar(); > > - if (ch == EOF) { > > + if (ttyEOF != 0x0 && ch == ttyEOF) { > > break; > > } > > readline_handle_byte(readline_state, ch); > > @@ -592,6 +615,7 @@ int main(int argc, char **argv) > > qemuio_add_command(&close_cmd); > > > > if (isatty(STDIN_FILENO)) { > > + ttyEOF = get_eof_char(); > > readline_state = readline_init(readline_printf_func, > > readline_flush_func, > > NULL, > > -- > > 2.14.3 > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > Regards, Daniel
Am 08.12.2017 um 14:34 hat Daniel P. Berrange geschrieben: > qemu-io puts the TTY into non-canonical mode, which means no EOF processing is > done and thus getchar() will never return the EOF constant. Instead we have to > query the TTY attributes to determine the configured EOF character (usually > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the > regression that prevented Ctrl-D from triggering an exit of qemu-io that has > existed since readline was first added in > > commit 0cf17e181798063c3824c8200ba46f25f54faa1a > Author: Stefan Hajnoczi <stefanha@redhat.com> > Date: Thu Nov 14 11:54:17 2013 +0100 > > qemu-io: use readline.c > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Thanks, applied to the block branch. It would be nice if we also printed a newline when Ctrl-D is pressed. If you want to send a v3 for this, I'll just update the patch in my queue. Kevin
diff --git a/qemu-io.c b/qemu-io.c index c70bde3eb1..fa4972d734 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -10,6 +10,9 @@ #include "qemu/osdep.h" #include <getopt.h> #include <libgen.h> +#ifndef _WIN32 +#include <termios.h> +#endif #include "qapi/error.h" #include "qemu-io.h" @@ -41,6 +44,26 @@ static bool imageOpts; static ReadLineState *readline_state; +static int ttyEOF; + +static int get_eof_char(void) +{ +#ifdef _WIN32 + return 0x4; /* Ctrl-D */ +#else + struct termios tty; + if (tcgetattr(STDIN_FILENO, &tty) != 0) { + if (errno == ENOTTY) { + return 0x0; /* just expect read() == 0 */ + } else { + return 0x4; /* Ctrl-D */ + } + } + + return tty.c_cc[VEOF]; +#endif +} + static int close_f(BlockBackend *blk, int argc, char **argv) { blk_unref(qemuio_blk); @@ -322,7 +345,7 @@ static char *fetchline_readline(void) readline_start(readline_state, get_prompt(), 0, readline_func, &line); while (!line) { int ch = getchar(); - if (ch == EOF) { + if (ttyEOF != 0x0 && ch == ttyEOF) { break; } readline_handle_byte(readline_state, ch); @@ -592,6 +615,7 @@ int main(int argc, char **argv) qemuio_add_command(&close_cmd); if (isatty(STDIN_FILENO)) { + ttyEOF = get_eof_char(); readline_state = readline_init(readline_printf_func, readline_flush_func, NULL,
qemu-io puts the TTY into non-canonical mode, which means no EOF processing is done and thus getchar() will never return the EOF constant. Instead we have to query the TTY attributes to determine the configured EOF character (usually Ctrl-D / 0x4), and then explicitly check for that value. This fixes the regression that prevented Ctrl-D from triggering an exit of qemu-io that has existed since readline was first added in commit 0cf17e181798063c3824c8200ba46f25f54faa1a Author: Stefan Hajnoczi <stefanha@redhat.com> Date: Thu Nov 14 11:54:17 2013 +0100 qemu-io: use readline.c Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- Changed in v2: - Query termios settings for EOF character qemu-io.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)