[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] chardev: add path option for pty backend
|
From: |
Paulo Neves |
|
Subject: |
Re: [PATCH v2] chardev: add path option for pty backend |
|
Date: |
Tue, 04 Jun 2024 09:28:44 +0000 |
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
>>