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.