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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, May 31, 2024 at 10:51:53AM -0700, Octavian Purdila wrote:
> Add path option to the pty char backend which will create a symbolic
> link to the given path that points to the allocated PTY.
> 
> 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: Octavian Purdila <tavip@google.com>

When creating a new version of someone else's previous patch, you
generally should keep the original Signed-off-by, then add 1 or 2
lines explaining what further changes you made, then add your own
Signed-off-by.


> ---
>  chardev/char-pty.c | 34 ++++++++++++++++++++++++++++++++++
>  chardev/char.c     |  5 +++++
>  qapi/char.json     |  4 ++--
>  qemu-options.hx    | 14 ++++++++++----
>  4 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index cc2f7617fe..7cd5d34851 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,37 @@ 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");
> +            close(master_fd);
> +        } 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 = path ? g_strdup(path) : NULL;
>  }
>  
>  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..5eec194242 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,16 @@ The available backends are:
>  
>      ``path`` specifies the name of the serial device to open.
>  
> -``-chardev pty,id=id``
> +``-chardev pty,id=id[,path=path]``
>      Create a new pseudo-terminal on the host and connect to it. ``pty``
>      does not take any options.
>  
>      ``pty`` is not available on Windows hosts.
>  
> +    ``path`` specifies the symbolic link path to be created that
> +    points to the pty device.
> +
> +
>  ``-chardev stdio,id=id[,signal=on|off]``
>      Connect to standard input and standard output of the QEMU process.
>  
> @@ -4171,8 +4175,10 @@ 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).
> +        Optionally a path can be given to create a symbolic link to
> +        the allocated PTY.
>  
>      ``none``
>          No device is allocated. Note that for machine types which
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 
> 

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