[PATCH v3] chardev: add path option for pty backend

[PATCH v3] chardev: add path option for pty backend

[Octavian Purdila]

Add path option to the pty char backend which will create a symbolic
link to the given path that points to the allocated PTY.

This avoids having to make QMP or HMP monitor queries to find out what
the new PTY device path is.

Based on patch from Paulo Neves:

https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/

Tested with the following invocations that the link is created and
removed when qemu stops:

  qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
  -chardev pty,path=test,id=compat_monitor0

  qemu-system-x86_64 -nodefaults -monitor pty:test

Also tested that when a link path is not passed invocations still work, e.g.:

  qemu-system-x86_64 -monitor pty

Co-authored-by: Paulo Neves <ptsneves@gmail.com>
Signed-off-by: Paulo Neves <ptsneves@gmail.com>
[OP: rebase and address original patch review comments]
Signed-off-by: Octavian Purdila <tavip@google.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
Changes since v2:

 * remove NULL path check, g_strdup() allows NULL inputs  

Changes since v1:

 * Keep the original Signed-off-by from Paulo and add one line
    description with further changes

 * Update commit message with justification for why the new
    functionality is useful

 * Don't close master_fd when symlink creation fails to avoid double
    close

 * Update documentation for clarity

chardev/char-pty.c | 33 +++++++++++++++++++++++++++++++++
 chardev/char.c     |  5 +++++
 qapi/char.json     |  4 ++--
 qemu-options.hx    | 24 ++++++++++++++++++------
 4 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index cc2f7617fe..5c6172ddba 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -29,6 +29,7 @@
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
 #include "qemu/qemu-print.h"
 
 #include "chardev/char-io.h"
@@ -41,6 +42,7 @@ struct PtyChardev {
 
     int connected;
     GSource *timer_src;
+    char *symlink_path;
 };
 typedef struct PtyChardev PtyChardev;
 
@@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     PtyChardev *s = PTY_CHARDEV(obj);
 
+    /* unlink symlink */
+    if (s->symlink_path) {
+        unlink(s->symlink_path);
+        g_free(s->symlink_path);
+    }
+
     pty_chr_state(chr, 0);
     object_unref(OBJECT(s->ioc));
     pty_chr_timer_cancel(s);
@@ -330,6 +338,7 @@ static void char_pty_open(Chardev *chr,
     int master_fd, slave_fd;
     char pty_name[PATH_MAX];
     char *name;
+    char *symlink_path = backend->u.pty.data->device;
 
     master_fd = qemu_openpty_raw(&slave_fd, pty_name);
     if (master_fd < 0) {
@@ -354,12 +363,36 @@ static void char_pty_open(Chardev *chr,
     g_free(name);
     s->timer_src = NULL;
     *be_opened = false;
+
+    /* create symbolic link */
+    if (symlink_path) {
+        int res = symlink(pty_name, symlink_path);
+
+        if (res != 0) {
+            error_setg_errno(errp, errno, "Failed to create PTY symlink");
+        } else {
+            s->symlink_path = g_strdup(symlink_path);
+        }
+    }
+}
+
+static void char_pty_parse(QemuOpts *opts, ChardevBackend *backend,
+                           Error **errp)
+{
+    const char *path = qemu_opt_get(opts, "path");
+    ChardevHostdev *dev;
+
+    backend->type = CHARDEV_BACKEND_KIND_PTY;
+    dev = backend->u.pty.data = g_new0(ChardevHostdev, 1);
+    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(dev));
+    dev->device = g_strdup(path);
 }
 
 static void char_pty_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
+    cc->parse = char_pty_parse;
     cc->open = char_pty_open;
     cc->chr_write = char_pty_chr_write;
     cc->chr_update_read_handler = pty_chr_update_read_handler;
diff --git a/chardev/char.c b/chardev/char.c
index 3c43fb1278..404c6b8a4f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -428,6 +428,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
         qemu_opt_set(opts, "path", p, &error_abort);
         return opts;
     }
+    if (strstart(filename, "pty:", &p)) {
+        qemu_opt_set(opts, "backend", "pty", &error_abort);
+        qemu_opt_set(opts, "path", p, &error_abort);
+        return opts;
+    }
     if (strstart(filename, "tcp:", &p) ||
         strstart(filename, "telnet:", &p) ||
         strstart(filename, "tn3270:", &p) ||
diff --git a/qapi/char.json b/qapi/char.json
index 777dde55d9..4c74bfc437 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -509,7 +509,7 @@
 ##
 # @ChardevHostdevWrapper:
 #
-# @data: Configuration info for device and pipe chardevs
+# @data: Configuration info for device, pty and pipe chardevs
 #
 # Since: 1.4
 ##
@@ -650,7 +650,7 @@
             'pipe': 'ChardevHostdevWrapper',
             'socket': 'ChardevSocketWrapper',
             'udp': 'ChardevUdpWrapper',
-            'pty': 'ChardevCommonWrapper',
+            'pty': 'ChardevHostdevWrapper',
             'null': 'ChardevCommonWrapper',
             'mux': 'ChardevMuxWrapper',
             'msmouse': 'ChardevCommonWrapper',
diff --git a/qemu-options.hx b/qemu-options.hx
index 8ca7f34ef0..94ffb1a605 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3569,7 +3569,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
     "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
 #else
-    "-chardev pty,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
+    "-chardev pty,id=id[,path=path][,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
     "-chardev stdio,id=id[,mux=on|off][,signal=on|off][,logfile=PATH][,logappend=on|off]\n"
 #endif
 #ifdef CONFIG_BRLAPI
@@ -3808,12 +3808,18 @@ The available backends are:
 
     ``path`` specifies the name of the serial device to open.
 
-``-chardev pty,id=id``
-    Create a new pseudo-terminal on the host and connect to it. ``pty``
-    does not take any options.
+``-chardev pty,id=id[,path=path]``
+    Create a new pseudo-terminal on the host and connect to it.
 
     ``pty`` is not available on Windows hosts.
 
+    If ``path`` is specified, QEMU will create a symbolic link at
+    that location which points to the new PTY device.
+
+    This avoids having to make QMP or HMP monitor queries to find out
+    what the new PTY device path is.
+
+
 ``-chardev stdio,id=id[,signal=on|off]``
     Connect to standard input and standard output of the QEMU process.
 
@@ -4171,8 +4177,14 @@ SRST
 
             vc:80Cx24C
 
-    ``pty``
-        [Linux only] Pseudo TTY (a new PTY is automatically allocated)
+    ``pty[:path]``
+        [Linux only] Pseudo TTY (a new PTY is automatically allocated).
+
+        If ``path`` is specified, QEMU will create a symbolic link at
+        that location which points to the new PTY device.
+
+        This avoids having to make QMP or HMP monitor queries to find
+        out what the new PTY device path is.
 
     ``none``
         No device is allocated. Note that for machine types which
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH v3] chardev: add path option for pty backend

[Markus Armbruster]

Looks like this one fell through the cracks.

Octavian Purdila <tavip@google.com> writes:

> Add path option to the pty char backend which will create a symbolic
> link to the given path that points to the allocated PTY.
>
> This avoids having to make QMP or HMP monitor queries to find out what
> the new PTY device path is.

QMP commands chardev-add and chardev-change return the information you
want:

    # @pty: name of the slave pseudoterminal device, present if and only
    #     if a chardev of type 'pty' was created

So does HMP command chardev-add.  HMP chardev apparently doesn't, but
that could be fixed.

So, the use case is basically the command line, right?

> Based on patch from Paulo Neves:
>
> https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/
>
> Tested with the following invocations that the link is created and
> removed when qemu stops:
>
>   qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
>   -chardev pty,path=test,id=compat_monitor0
>
>   qemu-system-x86_64 -nodefaults -monitor pty:test
>
> Also tested that when a link path is not passed invocations still work, e.g.:
>
>   qemu-system-x86_64 -monitor pty
>
> Co-authored-by: Paulo Neves <ptsneves@gmail.com>
> Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> [OP: rebase and address original patch review comments]
> Signed-off-by: Octavian Purdila <tavip@google.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> Changes since v2:
>
>  * remove NULL path check, g_strdup() allows NULL inputs  
>
> Changes since v1:
>
>  * Keep the original Signed-off-by from Paulo and add one line
>     description with further changes
>
>  * Update commit message with justification for why the new
>     functionality is useful
>
>  * Don't close master_fd when symlink creation fails to avoid double
>     close
>
>  * Update documentation for clarity
>
> chardev/char-pty.c | 33 +++++++++++++++++++++++++++++++++
>  chardev/char.c     |  5 +++++
>  qapi/char.json     |  4 ++--
>  qemu-options.hx    | 24 ++++++++++++++++++------
>  4 files changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index cc2f7617fe..5c6172ddba 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -29,6 +29,7 @@
>  #include "qemu/sockets.h"
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
> +#include "qemu/option.h"
>  #include "qemu/qemu-print.h"
>  
>  #include "chardev/char-io.h"
> @@ -41,6 +42,7 @@ struct PtyChardev {
>  
>      int connected;
>      GSource *timer_src;
> +    char *symlink_path;
>  };
>  typedef struct PtyChardev PtyChardev;
>  
> @@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
>      Chardev *chr = CHARDEV(obj);
>      PtyChardev *s = PTY_CHARDEV(obj);
>  
> +    /* unlink symlink */
> +    if (s->symlink_path) {
> +        unlink(s->symlink_path);
> +        g_free(s->symlink_path);
> +    }

Runs when the chardev object is finalized.

Doesn't run when QEMU crashes.  Stale symlink left behind then.  Can't
see how you could avoid that at reasonable cost.  Troublesome all the
same.

> +
>      pty_chr_state(chr, 0);
>      object_unref(OBJECT(s->ioc));
>      pty_chr_timer_cancel(s);
> @@ -330,6 +338,7 @@ static void char_pty_open(Chardev *chr,
>      int master_fd, slave_fd;
>      char pty_name[PATH_MAX];
>      char *name;
> +    char *symlink_path = backend->u.pty.data->device;
>  
>      master_fd = qemu_openpty_raw(&slave_fd, pty_name);
>      if (master_fd < 0) {
> @@ -354,12 +363,36 @@ static void char_pty_open(Chardev *chr,
>      g_free(name);
>      s->timer_src = NULL;
>      *be_opened = false;
> +
> +    /* create symbolic link */
> +    if (symlink_path) {
> +        int res = symlink(pty_name, symlink_path);
> +
> +        if (res != 0) {
> +            error_setg_errno(errp, errno, "Failed to create PTY symlink");
> +        } else {
> +            s->symlink_path = g_strdup(symlink_path);
> +        }
> +    }
> +}
> +
> +static void char_pty_parse(QemuOpts *opts, ChardevBackend *backend,
> +                           Error **errp)
> +{
> +    const char *path = qemu_opt_get(opts, "path");
> +    ChardevHostdev *dev;
> +
> +    backend->type = CHARDEV_BACKEND_KIND_PTY;
> +    dev = backend->u.pty.data = g_new0(ChardevHostdev, 1);
> +    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(dev));
> +    dev->device = g_strdup(path);

Put a pin into this line.

>  }
>  
>  static void char_pty_class_init(ObjectClass *oc, void *data)
>  {
>      ChardevClass *cc = CHARDEV_CLASS(oc);
>  
> +    cc->parse = char_pty_parse;
>      cc->open = char_pty_open;
>      cc->chr_write = char_pty_chr_write;
>      cc->chr_update_read_handler = pty_chr_update_read_handler;
> diff --git a/chardev/char.c b/chardev/char.c
> index 3c43fb1278..404c6b8a4f 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -428,6 +428,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>          qemu_opt_set(opts, "path", p, &error_abort);
>          return opts;
>      }
> +    if (strstart(filename, "pty:", &p)) {
> +        qemu_opt_set(opts, "backend", "pty", &error_abort);
> +        qemu_opt_set(opts, "path", p, &error_abort);
> +        return opts;
> +    }
>      if (strstart(filename, "tcp:", &p) ||
>          strstart(filename, "telnet:", &p) ||
>          strstart(filename, "tn3270:", &p) ||
> diff --git a/qapi/char.json b/qapi/char.json
> index 777dde55d9..4c74bfc437 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -509,7 +509,7 @@
>  ##
>  # @ChardevHostdevWrapper:
>  #
> -# @data: Configuration info for device and pipe chardevs
> +# @data: Configuration info for device, pty and pipe chardevs
>  #
>  # Since: 1.4
>  ##
   { 'struct': 'ChardevHostdev',
     'data': { 'device': 'str' },
     'base': 'ChardevCommon' }
> @@ -650,7 +650,7 @@
>              'pipe': 'ChardevHostdevWrapper',
>              'socket': 'ChardevSocketWrapper',
>              'udp': 'ChardevUdpWrapper',
> -            'pty': 'ChardevCommonWrapper',
> +            'pty': 'ChardevHostdevWrapper',

This adds member @device.

The fact that @device is since 9.1 remains undocumented.

In HMP and CLI, the new option is called "path".

In QMP, it's called "device".  Not only is this inconsistent (see the
pin above), it's misleading: it's not a device name, it's the name of a
symbolic link pointing to a slave PTY device file.

>              'null': 'ChardevCommonWrapper',
>              'mux': 'ChardevMuxWrapper',
>              'msmouse': 'ChardevCommonWrapper',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8ca7f34ef0..94ffb1a605 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3569,7 +3569,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>      "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>      "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>  #else
> -    "-chardev pty,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> +    "-chardev pty,id=id[,path=path][,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>      "-chardev stdio,id=id[,mux=on|off][,signal=on|off][,logfile=PATH][,logappend=on|off]\n"
>  #endif
>  #ifdef CONFIG_BRLAPI
> @@ -3808,12 +3808,18 @@ The available backends are:
>  
>      ``path`` specifies the name of the serial device to open.
>  
> -``-chardev pty,id=id``
> -    Create a new pseudo-terminal on the host and connect to it. ``pty``
> -    does not take any options.
> +``-chardev pty,id=id[,path=path]``
> +    Create a new pseudo-terminal on the host and connect to it.
>  
>      ``pty`` is not available on Windows hosts.
>  
> +    If ``path`` is specified, QEMU will create a symbolic link at
> +    that location which points to the new PTY device.
> +
> +    This avoids having to make QMP or HMP monitor queries to find out
> +    what the new PTY device path is.
> +
> +
>  ``-chardev stdio,id=id[,signal=on|off]``
>      Connect to standard input and standard output of the QEMU process.
>  
> @@ -4171,8 +4177,14 @@ SRST
>  
>              vc:80Cx24C
>  
> -    ``pty``
> -        [Linux only] Pseudo TTY (a new PTY is automatically allocated)
> +    ``pty[:path]``
> +        [Linux only] Pseudo TTY (a new PTY is automatically allocated).
> +
> +        If ``path`` is specified, QEMU will create a symbolic link at
> +        that location which points to the new PTY device.
> +
> +        This avoids having to make QMP or HMP monitor queries to find
> +        out what the new PTY device path is.
>  
>      ``none``
>          No device is allocated. Note that for machine types which

Sure we want to extend the old-style syntax?  I vaguely recall we
already have certain configuration knobs that are unavailable there.


The feature feels rather doubtful to me, to be honest.

Re: [PATCH v3] chardev: add path option for pty backend

[Peter Maydell]

On Thu, 18 Jul 2024 at 07:15, Markus Armbruster <armbru@redhat.com> wrote:
>
> Looks like this one fell through the cracks.
>
> Octavian Purdila <tavip@google.com> writes:
>
> > Add path option to the pty char backend which will create a symbolic
> > link to the given path that points to the allocated PTY.
> >
> > This avoids having to make QMP or HMP monitor queries to find out what
> > the new PTY device path is.
>
> QMP commands chardev-add and chardev-change return the information you
> want:
>
>     # @pty: name of the slave pseudoterminal device, present if and only
>     #     if a chardev of type 'pty' was created
>
> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
> that could be fixed.
>
> So, the use case is basically the command line, right?

> The feature feels rather doubtful to me, to be honest.

The command line is an important use-case, though. Not every
user of QEMU is libvirt with a QMP/HMP connection readily
to hand that they would prefer to use for all configuration...

-- PMM

Re: [PATCH v3] chardev: add path option for pty backend

[Daniel P. Berrangé]

On Thu, Jul 18, 2024 at 08:15:01AM +0200, Markus Armbruster wrote:
> Looks like this one fell through the cracks.
> 
> Octavian Purdila <tavip@google.com> writes:
> 
> > Add path option to the pty char backend which will create a symbolic
> > link to the given path that points to the allocated PTY.
> >
> > This avoids having to make QMP or HMP monitor queries to find out what
> > the new PTY device path is.
> 
> QMP commands chardev-add and chardev-change return the information you
> want:
> 
>     # @pty: name of the slave pseudoterminal device, present if and only
>     #     if a chardev of type 'pty' was created
> 
> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
> that could be fixed.

It does print it:

  (qemu) chardev-add  pty,id=bar
  char device redirected to /dev/pts/12 (label bar)


> So, the use case is basically the command line, right?

Also cli prints it

  $ qemu-system-x86_64 -chardev pty,id=foo -monitor stdio -display none
  char device redirected to /dev/pts/10 (label foo)


> > Based on patch from Paulo Neves:
> >
> > https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/
> >
> > Tested with the following invocations that the link is created and
> > removed when qemu stops:
> >
> >   qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
> >   -chardev pty,path=test,id=compat_monitor0
> >
> >   qemu-system-x86_64 -nodefaults -monitor pty:test
> >
> > Also tested that when a link path is not passed invocations still work, e.g.:
> >
> >   qemu-system-x86_64 -monitor pty
> >
> > Co-authored-by: Paulo Neves <ptsneves@gmail.com>
> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> > [OP: rebase and address original patch review comments]
> > Signed-off-by: Octavian Purdila <tavip@google.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > Changes since v2:
> >
> >  * remove NULL path check, g_strdup() allows NULL inputs  
> >
> > Changes since v1:
> >
> >  * Keep the original Signed-off-by from Paulo and add one line
> >     description with further changes
> >
> >  * Update commit message with justification for why the new
> >     functionality is useful
> >
> >  * Don't close master_fd when symlink creation fails to avoid double
> >     close
> >
> >  * Update documentation for clarity
> >
> > chardev/char-pty.c | 33 +++++++++++++++++++++++++++++++++
> >  chardev/char.c     |  5 +++++
> >  qapi/char.json     |  4 ++--
> >  qemu-options.hx    | 24 ++++++++++++++++++------
> >  4 files changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index cc2f7617fe..5c6172ddba 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -29,6 +29,7 @@
> >  #include "qemu/sockets.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> > +#include "qemu/option.h"
> >  #include "qemu/qemu-print.h"
> >  
> >  #include "chardev/char-io.h"
> > @@ -41,6 +42,7 @@ struct PtyChardev {
> >  
> >      int connected;
> >      GSource *timer_src;
> > +    char *symlink_path;
> >  };
> >  typedef struct PtyChardev PtyChardev;
> >  
> > @@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
> >      Chardev *chr = CHARDEV(obj);
> >      PtyChardev *s = PTY_CHARDEV(obj);
> >  
> > +    /* unlink symlink */
> > +    if (s->symlink_path) {
> > +        unlink(s->symlink_path);
> > +        g_free(s->symlink_path);
> > +    }
> 
> Runs when the chardev object is finalized.
> 
> Doesn't run when QEMU crashes.  Stale symlink left behind then.  Can't
> see how you could avoid that at reasonable cost.  Troublesome all the
> same.

Do we ever guarantee that the finalizer runs ?  eg dif we have

  error_setg(&error_exit, ....

that's a clean exit, not a crash, but I don't think chardev finalizers
will run, as we don't do atexit() hooks for it.


> The feature feels rather doubtful to me, to be honest.

On the one hand I understand the pain - long ago libvirt had to deal
with parsing the console messages

  char device redirected to /dev/pts/10 (label foo)

before we switched to using QMP to query this.

On the other hand, in retrospect libvirt should never have used the 'pty'
backend in the first place. The 'unix' socket backend is a  choice as it
has predictable filenames, and it has proper connection oriented semantics,
so QEMU can reliably detect when clients disconnect, which has always been
troublesome for the 'pty' backend.

So while I can understand the desire to add a 'path' option to 'pty'
to trigger symlink creation, I think we could choose to tell people
to use the 'unix' socket backend instead if they want a predictable
path. This would avoid us creating the difficult to fix bug for
symlink deletion in error conditions.

What's the key benefit of the 'pty' backend, that 'unix' doesn't
handle ?

With 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: [PATCH v3] chardev: add path option for pty backend

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 18 Jul 2024 at 07:15, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Looks like this one fell through the cracks.
>>
>> Octavian Purdila <tavip@google.com> writes:
>>
>> > Add path option to the pty char backend which will create a symbolic
>> > link to the given path that points to the allocated PTY.
>> >
>> > This avoids having to make QMP or HMP monitor queries to find out what
>> > the new PTY device path is.
>>
>> QMP commands chardev-add and chardev-change return the information you
>> want:
>>
>>     # @pty: name of the slave pseudoterminal device, present if and only
>>     #     if a chardev of type 'pty' was created
>>
>> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
>> that could be fixed.
>>
>> So, the use case is basically the command line, right?
>
>> The feature feels rather doubtful to me, to be honest.
>
> The command line is an important use-case, though. Not every
> user of QEMU is libvirt with a QMP/HMP connection readily
> to hand that they would prefer to use for all configuration...

In general yes.  But what are the use cases for this one?

To me, specifying path=/mumble/symlink plus the bother of cleaning up
stale ones doesn't feel like much of an improvement over reading the pty
name from "info chardev".  I guess I'm missing something.  Tell me!

If we decide we want this, then the QMP interface needs to be fixed:
Call the argument @path for consistency, and document it properly.
Actually straightforward, just create a new struct instead of pressing
ChardevHostdev into service.

Some advice on robust use of @path could be useful, in particular on
guarding against QEMU leaving stale links behind.

Additional decision: whether to extend the old-style syntax.

Re: [PATCH v3] chardev: add path option for pty backend

[Paulo Neves]

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

Peter Maydell [<peter.maydell@linaro.org>](mailto:peter.maydell@linaro.org) writes:

>> On Thu, 18 Jul 2024 at 07:15, Markus Armbruster
>> [<armbru@redhat.com>](mailto:armbru@redhat.com)
>> wrote:
>>
>>> Looks like this one fell through the cracks.
>>>
>>> Octavian Purdila
>>> [<tavip@google.com>](mailto:tavip@google.com)
>>> writes:
>>>
>>>> Add path option to the pty char backend which will create a symbolic
>>>> link to the given path that points to the allocated PTY.
>>>>
>>>> This avoids having to make QMP or HMP monitor queries to find out what
>>>> the new PTY device path is.
>>>
>>> QMP commands chardev-add and chardev-change return the information you
>>> want:
>>>
>>>     # @pty: name of the slave pseudoterminal device, present if and only
>>>     #     if a chardev of type 'pty' was created
>>>
>>> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
>>> that could be fixed.
>>>
>>> So, the use case is basically the command line, right?
>>
>>> The feature feels rather doubtful to me, to be honest.
>>
>> The command line is an important use-case, though. Not every
>> user of QEMU is libvirt with a QMP/HMP connection readily
>> to hand that they would prefer to use for all configuration...
>
> In general yes.  But what are the use cases for this one?
>
> To me, specifying path=/mumble/symlink plus the bother of cleaning up
> stale ones doesn't feel like much of an improvement over reading the pty
> name from "info chardev".  I guess I'm missing something.  Tell me!
>
> If we decide we want this, then the QMP interface needs to be fixed:
> Call the argument @path for consistency, and document it properly.
> Actually straightforward, just create a new struct instead of pressing
> ChardevHostdev into service.

The original use case was not about reading the path but allowing the caller to set the symlink. The cleaning up and ergonomics of handling that path are besides the point of the patch. In my case it was a path I could define in configuration and let other services use that chardev. Other services that did not require knowledge of whether the PTY was real or from QEMU and thus did not know how to query it.

[-- Attachment #2: Type: text/html, Size: 3280 bytes --]

Re: [PATCH v3] chardev: add path option for pty backend

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jul 18, 2024 at 08:15:01AM +0200, Markus Armbruster wrote:
>> Looks like this one fell through the cracks.
>> 
>> Octavian Purdila <tavip@google.com> writes:
>> 
>> > Add path option to the pty char backend which will create a symbolic
>> > link to the given path that points to the allocated PTY.
>> >
>> > This avoids having to make QMP or HMP monitor queries to find out what
>> > the new PTY device path is.
>> 
>> QMP commands chardev-add and chardev-change return the information you
>> want:
>> 
>>     # @pty: name of the slave pseudoterminal device, present if and only
>>     #     if a chardev of type 'pty' was created
>> 
>> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
>> that could be fixed.
>
> It does print it:
>
>   (qemu) chardev-add  pty,id=bar
>   char device redirected to /dev/pts/12 (label bar)

I fat-fingered "HMP chardev-change".

>> So, the use case is basically the command line, right?
>
> Also cli prints it
>
>   $ qemu-system-x86_64 -chardev pty,id=foo -monitor stdio -display none
>   char device redirected to /dev/pts/10 (label foo)

Good enough for ad hoc use by humans.

Management applications should use QMP, which returns it.

I guess there's scripts in between.

>> > Based on patch from Paulo Neves:
>> >
>> > https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/
>> >
>> > Tested with the following invocations that the link is created and
>> > removed when qemu stops:
>> >
>> >   qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
>> >   -chardev pty,path=test,id=compat_monitor0
>> >
>> >   qemu-system-x86_64 -nodefaults -monitor pty:test
>> >
>> > Also tested that when a link path is not passed invocations still work, e.g.:
>> >
>> >   qemu-system-x86_64 -monitor pty
>> >
>> > Co-authored-by: Paulo Neves <ptsneves@gmail.com>
>> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
>> > [OP: rebase and address original patch review comments]
>> > Signed-off-by: Octavian Purdila <tavip@google.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[...]

>> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> > index cc2f7617fe..5c6172ddba 100644
>> > --- a/chardev/char-pty.c
>> > +++ b/chardev/char-pty.c
>> > @@ -29,6 +29,7 @@
>> >  #include "qemu/sockets.h"
>> >  #include "qemu/error-report.h"
>> >  #include "qemu/module.h"
>> > +#include "qemu/option.h"
>> >  #include "qemu/qemu-print.h"
>> >  
>> >  #include "chardev/char-io.h"
>> > @@ -41,6 +42,7 @@ struct PtyChardev {
>> >  
>> >      int connected;
>> >      GSource *timer_src;
>> > +    char *symlink_path;
>> >  };
>> >  typedef struct PtyChardev PtyChardev;
>> >  
>> > @@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
>> >      Chardev *chr = CHARDEV(obj);
>> >      PtyChardev *s = PTY_CHARDEV(obj);
>> >  
>> > +    /* unlink symlink */
>> > +    if (s->symlink_path) {
>> > +        unlink(s->symlink_path);
>> > +        g_free(s->symlink_path);
>> > +    }
>> 
>> Runs when the chardev object is finalized.
>> 
>> Doesn't run when QEMU crashes.  Stale symlink left behind then.  Can't
>> see how you could avoid that at reasonable cost.  Troublesome all the
>> same.
>
> Do we ever guarantee that the finalizer runs ?  eg dif we have
>
>   error_setg(&error_exit, ....
>
> that's a clean exit, not a crash, but I don't think chardev finalizers
> will run, as we don't do atexit() hooks for it.

Point.

>> The feature feels rather doubtful to me, to be honest.
>
> On the one hand I understand the pain - long ago libvirt had to deal
> with parsing the console messages
>
>   char device redirected to /dev/pts/10 (label foo)
>
> before we switched to using QMP to query this.
>
> On the other hand, in retrospect libvirt should never have used the 'pty'
> backend in the first place. The 'unix' socket backend is a  choice as it
> has predictable filenames, and it has proper connection oriented semantics,
> so QEMU can reliably detect when clients disconnect, which has always been
> troublesome for the 'pty' backend.
>
> So while I can understand the desire to add a 'path' option to 'pty'
> to trigger symlink creation, I think we could choose to tell people
> to use the 'unix' socket backend instead if they want a predictable
> path. This would avoid us creating the difficult to fix bug for
> symlink deletion in error conditions.
>
> What's the key benefit of the 'pty' backend, that 'unix' doesn't
> handle ?

I think this is the question to answer.

Re: [PATCH v3] chardev: add path option for pty backend

[Octavian Purdila]

On Thu, Jul 18, 2024 at 3:22 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Thu, Jul 18, 2024 at 08:15:01AM +0200, Markus Armbruster wrote:
> >> Looks like this one fell through the cracks.
> >>
> >> Octavian Purdila <tavip@google.com> writes:
> >>
> >> > Add path option to the pty char backend which will create a symbolic
> >> > link to the given path that points to the allocated PTY.
> >> >
> >> > This avoids having to make QMP or HMP monitor queries to find out what
> >> > the new PTY device path is.
> >>
> >> QMP commands chardev-add and chardev-change return the information you
> >> want:
> >>
> >>     # @pty: name of the slave pseudoterminal device, present if and only
> >>     #     if a chardev of type 'pty' was created
> >>
> >> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
> >> that could be fixed.
> >
> > It does print it:
> >
> >   (qemu) chardev-add  pty,id=bar
> >   char device redirected to /dev/pts/12 (label bar)
>
> I fat-fingered "HMP chardev-change".
>
> >> So, the use case is basically the command line, right?
> >
> > Also cli prints it
> >
> >   $ qemu-system-x86_64 -chardev pty,id=foo -monitor stdio -display none
> >   char device redirected to /dev/pts/10 (label foo)
>
> Good enough for ad hoc use by humans.
>
> Management applications should use QMP, which returns it.
>
> I guess there's scripts in between.
>
> >> > Based on patch from Paulo Neves:
> >> >
> >> > https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsneves@gmail.com/
> >> >
> >> > Tested with the following invocations that the link is created and
> >> > removed when qemu stops:
> >> >
> >> >   qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
> >> >   -chardev pty,path=test,id=compat_monitor0
> >> >
> >> >   qemu-system-x86_64 -nodefaults -monitor pty:test
> >> >
> >> > Also tested that when a link path is not passed invocations still work, e.g.:
> >> >
> >> >   qemu-system-x86_64 -monitor pty
> >> >
> >> > Co-authored-by: Paulo Neves <ptsneves@gmail.com>
> >> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> >> > [OP: rebase and address original patch review comments]
> >> > Signed-off-by: Octavian Purdila <tavip@google.com>
> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> [...]
>
> >> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> >> > index cc2f7617fe..5c6172ddba 100644
> >> > --- a/chardev/char-pty.c
> >> > +++ b/chardev/char-pty.c
> >> > @@ -29,6 +29,7 @@
> >> >  #include "qemu/sockets.h"
> >> >  #include "qemu/error-report.h"
> >> >  #include "qemu/module.h"
> >> > +#include "qemu/option.h"
> >> >  #include "qemu/qemu-print.h"
> >> >
> >> >  #include "chardev/char-io.h"
> >> > @@ -41,6 +42,7 @@ struct PtyChardev {
> >> >
> >> >      int connected;
> >> >      GSource *timer_src;
> >> > +    char *symlink_path;
> >> >  };
> >> >  typedef struct PtyChardev PtyChardev;
> >> >
> >> > @@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
> >> >      Chardev *chr = CHARDEV(obj);
> >> >      PtyChardev *s = PTY_CHARDEV(obj);
> >> >
> >> > +    /* unlink symlink */
> >> > +    if (s->symlink_path) {
> >> > +        unlink(s->symlink_path);
> >> > +        g_free(s->symlink_path);
> >> > +    }
> >>
> >> Runs when the chardev object is finalized.
> >>
> >> Doesn't run when QEMU crashes.  Stale symlink left behind then.  Can't
> >> see how you could avoid that at reasonable cost.  Troublesome all the
> >> same.
> >
> > Do we ever guarantee that the finalizer runs ?  eg dif we have
> >
> >   error_setg(&error_exit, ....
> >
> > that's a clean exit, not a crash, but I don't think chardev finalizers
> > will run, as we don't do atexit() hooks for it.
>
> Point.
>

I agree this is a shortcoming. But this can easily be fixed externally
at the invocation path.

> >> The feature feels rather doubtful to me, to be honest.
> >
> > On the one hand I understand the pain - long ago libvirt had to deal
> > with parsing the console messages
> >
> >   char device redirected to /dev/pts/10 (label foo)
> >
> > before we switched to using QMP to query this.
> >
> > On the other hand, in retrospect libvirt should never have used the 'pty'
> > backend in the first place. The 'unix' socket backend is a  choice as it
> > has predictable filenames, and it has proper connection oriented semantics,
> > so QEMU can reliably detect when clients disconnect, which has always been
> > troublesome for the 'pty' backend.
> >
> > So while I can understand the desire to add a 'path' option to 'pty'
> > to trigger symlink creation, I think we could choose to tell people
> > to use the 'unix' socket backend instead if they want a predictable
> > path. This would avoid us creating the difficult to fix bug for
> > symlink deletion in error conditions.
> >
> > What's the key benefit of the 'pty' backend, that 'unix' doesn't
> > handle ?
>
> I think this is the question to answer.
>

In my case the user of the serial device does not support unix sockets.

Re: [PATCH v3] chardev: add path option for pty backend

[Marc-André Lureau]

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

Hi Octavian,

You should send a new version with the changes requested by Markus. (we
might miss 9.1 though)

On Thu, Jul 18, 2024 at 1:48 PM Markus Armbruster <armbru@redhat.com> wrote:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 18 Jul 2024 at 07:15, Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> Looks like this one fell through the cracks.
> >>
> >> Octavian Purdila <tavip@google.com> writes:
> >>
> >> > Add path option to the pty char backend which will create a symbolic
> >> > link to the given path that points to the allocated PTY.
> >> >
> >> > This avoids having to make QMP or HMP monitor queries to find out what
> >> > the new PTY device path is.
> >>
> >> QMP commands chardev-add and chardev-change return the information you
> >> want:
> >>
> >>     # @pty: name of the slave pseudoterminal device, present if and only
> >>     #     if a chardev of type 'pty' was created
> >>
> >> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
> >> that could be fixed.
> >>
> >> So, the use case is basically the command line, right?
> >
> >> The feature feels rather doubtful to me, to be honest.
> >
> > The command line is an important use-case, though. Not every
> > user of QEMU is libvirt with a QMP/HMP connection readily
> > to hand that they would prefer to use for all configuration...
>
> In general yes.  But what are the use cases for this one?
>
> To me, specifying path=/mumble/symlink plus the bother of cleaning up
> stale ones doesn't feel like much of an improvement over reading the pty
> name from "info chardev".  I guess I'm missing something.  Tell me!
>
> If we decide we want this, then the QMP interface needs to be fixed:
> Call the argument @path for consistency, and document it properly.
> Actually straightforward, just create a new struct instead of pressing
> ChardevHostdev into service.
>
> Some advice on robust use of @path could be useful, in particular on
> guarding against QEMU leaving stale links behind.
>
> Additional decision: whether to extend the old-style syntax.
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3082 bytes --]