[PATCH] chardev: add path option for pty backend

[PATCH] 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.

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

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

[Daniel P. Berrangé]

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

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

[Marc-André Lureau]

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

Hi

On Sat, Jun 1, 2024 at 1:21 AM Octavian Purdila <tavip@google.com> 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>
> ---
>  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);
>

The fd is owned by s->ioc at this point: this will end up in double-close
in finalize.


> +        } 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;
> +    }
>

I am not sure we want to expand compat parsing here, but probably ok.


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

-- 
Marc-André Lureau

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

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

[Peter Maydell]

On Fri, 31 May 2024 at 22:21, Octavian Purdila <tavip@google.com> 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

Could we have some justification here for why the new
functionality is useful, please? (e.g. what new use cases
it permits).

> --- 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',

Does this break QAPI compatibility?

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

We just added an option, so we should delete the line saying
that it doesn't 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.

I think we could usefully make this a little less terse. Perhaps
   If ``path`` is specified, QEMU will create a symbolic link at
   that location which points to the new PTY device.
?

thanks
-- PMM

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

[Marc-André Lureau]

Hi

On Mon, Jun 3, 2024 at 4:23 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 31 May 2024 at 22:21, Octavian Purdila <tavip@google.com> 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
>
> Could we have some justification here for why the new
> functionality is useful, please? (e.g. what new use cases
> it permits).
>

It avoids the need to HMP/QMP query the allocated pty path. I don't
think there are other benefits.

> > --- 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',
>
> Does this break QAPI compatibility?
>
> > 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.
>
> We just added an option, so we should delete the line saying
> that it doesn't 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.
>
> I think we could usefully make this a little less terse. Perhaps
>    If ``path`` is specified, QEMU will create a symbolic link at
>    that location which points to the new PTY device.
> ?
>
> thanks
> -- PMM
>

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

[Daniel P. Berrangé]

On Mon, Jun 03, 2024 at 01:23:00PM +0100, Peter Maydell wrote:
> On Fri, 31 May 2024 at 22:21, Octavian Purdila <tavip@google.com> 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
> 
> Could we have some justification here for why the new
> functionality is useful, please? (e.g. what new use cases
> it permits).

The PTY paths are dynamically allocated so you can't predict them
as an app launching QEMU. You need to connect to the monitor and
interogate QEMU to find the path. So supporting a symlink simplifies
usage.

This was explained in the original patches' commit message, and
that description should have been kept.

> > --- 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',
> 
> Does this break QAPI compatibility?

No, what matters for compatibility is the *contents* of the
struct, not the particular struct names. Since ChardevHostdevWrapper
is a superset of of ChardevCommonWrapper we are fine with back compat.

> 
> > 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.
> 
> We just added an option, so we should delete the line saying
> that it doesn't 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.
> 
> I think we could usefully make this a little less terse. Perhaps
>    If ``path`` is specified, QEMU will create a symbolic link at
>    that location which points to the new PTY device.
> ?


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

[Peter Maydell]

On Mon, 3 Jun 2024 at 13:56, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 03, 2024 at 01:23:00PM +0100, Peter Maydell wrote:
> > On Fri, 31 May 2024 at 22:21, Octavian Purdila <tavip@google.com> 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
> >
> > Could we have some justification here for why the new
> > functionality is useful, please? (e.g. what new use cases
> > it permits).
>
> The PTY paths are dynamically allocated so you can't predict them
> as an app launching QEMU. You need to connect to the monitor and
> interogate QEMU to find the path. So supporting a symlink simplifies
> usage.
>
> This was explained in the original patches' commit message, and
> that description should have been kept.

Sounds good. We could add that to the documentation also:

> > >      ``pty`` is not available on Windows hosts.
> > >
> > > +    ``path`` specifies the symbolic link path to be created that
> > > +    points to the pty device.
> >
> > I think we could usefully make this a little less terse. Perhaps
> >    If ``path`` is specified, QEMU will create a symbolic link at
> >    that location which points to the new PTY device.
> > ?

       This allows you to avoid having to make a query via the QMP
       or HMP monitor to find out what the new PTY device path is.

thanks
-- PMM