On Thu, Jul 18, 2024 at 3:22 AM Markus Armbruster <arm...@redhat.com> wrote: > > Daniel P. Berrangé <berra...@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 <ta...@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-ptsne...@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 <ptsne...@gmail.com> > >> > Signed-off-by: Paulo Neves <ptsne...@gmail.com> > >> > [OP: rebase and address original patch review comments] > >> > Signed-off-by: Octavian Purdila <ta...@google.com> > >> > Reviewed-by: Marc-André Lureau <marcandre.lur...@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.
