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

[Daniel P . Berrangé]

On Thu, Jul 18, 2024 at 08:15:01AM +0200, Markus Armbruster wrote:
> Looks like this one fell through the cracks.
> 
> Octavian Purdila <tavip@google.com> writes:
> 
> > 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.
> 
> QMP commands chardev-add and chardev-change return the information you
> want:
> 
>     # @pty: name of the slave pseudoterminal device, present if and only
>     #     if a chardev of type 'pty' was created
> 
> So does HMP command chardev-add.  HMP chardev apparently doesn't, but
> that could be fixed.

It does print it:

  (qemu) chardev-add  pty,id=bar
  char device redirected to /dev/pts/12 (label bar)


> So, the use case is basically the command line, right?

Also cli prints it

  $ qemu-system-x86_64 -chardev pty,id=foo -monitor stdio -display none
  char device redirected to /dev/pts/10 (label foo)


> > 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 v2:
> >
> >  * remove NULL path check, g_strdup() allows NULL inputs  
> >
> > 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..5c6172ddba 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);
> > +    }
> 
> Runs when the chardev object is finalized.
> 
> Doesn't run when QEMU crashes.  Stale symlink left behind then.  Can't
> see how you could avoid that at reasonable cost.  Troublesome all the
> same.

Do we ever guarantee that the finalizer runs ?  eg dif we have

  error_setg(&error_exit, ....

that's a clean exit, not a crash, but I don't think chardev finalizers
will run, as we don't do atexit() hooks for it.


> The feature feels rather doubtful to me, to be honest.

On the one hand I understand the pain - long ago libvirt had to deal
with parsing the console messages

  char device redirected to /dev/pts/10 (label foo)

before we switched to using QMP to query this.

On the other hand, in retrospect libvirt should never have used the 'pty'
backend in the first place. The 'unix' socket backend is a  choice as it
has predictable filenames, and it has proper connection oriented semantics,
so QEMU can reliably detect when clients disconnect, which has always been
troublesome for the 'pty' backend.

So while I can understand the desire to add a 'path' option to 'pty'
to trigger symlink creation, I think we could choose to tell people
to use the 'unix' socket backend instead if they want a predictable
path. This would avoid us creating the difficult to fix bug for
symlink deletion in error conditions.

What's the key benefit of the 'pty' backend, that 'unix' doesn't
handle ?

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