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

["Daniel P. Berrangé" <berrange@redhat.com>]

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