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

Octavian Purdila posted 1 patch 5 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240603212228.3878836-1-tavip@google.com
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
chardev/char-pty.c | 33 +++++++++++++++++++++++++++++++++
chardev/char.c     |  5 +++++
qapi/char.json     |  4 ++--
qemu-options.hx    | 24 ++++++++++++++++++------
4 files changed, 58 insertions(+), 8 deletions(-)
[PATCH v2] chardev: add path option for pty backend
Posted by Octavian Purdila 5 months, 3 weeks ago
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>
---
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..b5a4eb59fc 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 = 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..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.288.g0e0cd299f1-goog
Re: [PATCH v2] chardev: add path option for pty backend
Posted by Marc-André Lureau 5 months, 3 weeks ago
On Tue, Jun 4, 2024 at 1:22 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.
>
> 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>
> ---
> 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..b5a4eb59fc 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 = path ? g_strdup(path) : NULL;

minor nit, g_strdup(NULL) returns NULL. Get rid of "?:" if you send a v3.

>  }
>
>  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.288.g0e0cd299f1-goog
>
Re: [PATCH v2] chardev: add path option for pty backend
Posted by Paulo Neves 5 months, 3 weeks ago
Really happy somebody found the patch useful. I forgot to look at the 
review comments at the time
and never got it merged.
@Octavian thanks for bringing it up to shape.

Cheers
Paulo Neves

On 04/06/2024 07:50, Marc-André Lureau wrote:
> On Tue, Jun 4, 2024 at 1:22 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.
>>
>> 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>
>> ---
>> 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..b5a4eb59fc 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 = path ? g_strdup(path) : NULL;
> minor nit, g_strdup(NULL) returns NULL. Get rid of "?:" if you send a v3.
Indeed the original patch did not have the ternary.
>>   }
>>
>>   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.288.g0e0cd299f1-goog
>>
Re: [PATCH v2] chardev: add path option for pty backend
Posted by Marc-André Lureau 5 months, 3 weeks ago
On Tue, Jun 4, 2024 at 1:22 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.
>
> 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 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..b5a4eb59fc 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 = 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..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.288.g0e0cd299f1-goog
>