| Message ID | 20240531175153.2716309-1-tavip@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: add path option for pty backend | expand |
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
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
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
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 >
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
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
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
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(-)