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

[Octavian Purdila]

On Thu, Jul 18, 2024 at 3:22 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > 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)
>
> I fat-fingered "HMP chardev-change".
>
> >> 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)
>
> Good enough for ad hoc use by humans.
>
> Management applications should use QMP, which returns it.
>
> I guess there's scripts in between.
>
> >> > 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>
>
> [...]
>
> >> > 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.
>
> Point.
>

I agree this is a shortcoming. But this can easily be fixed externally
at the invocation path.

> >> 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 ?
>
> I think this is the question to answer.
>

In my case the user of the serial device does not support unix sockets.