[Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Daniel P. Berrange]

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

Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Daniel P. Berrange]

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Daniel P. Berrangé]

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

[Kevin Wolf]

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