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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

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
>