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

[Markus Armbruster <armbru@redhat.com>]

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.

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