[PATCH] RFC: char: deprecate usage of bidirectional pipe

[Daniel P. Berrangé]

On Fri, Aug 05, 2022 at 01:55:41PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jul 26, 2022 at 12:44 PM Daniel P. Berrangé <berrange at redhat.com> wrote:
> >
> > On Tue, Jul 26, 2022 at 12:32:32PM +0400, marcandre.lureau at redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > As Ed Swierk explained back in 2006:
> > > https://lists.nongnu.org/archive/html/qemu-devel/2006-12/msg00160.html
> > >
> > > "When qemu writes into the pipe, it immediately reads back what it just
> > > wrote and treats it as a monitor command, endlessly breathing its own
> > > exhaust."
> > >
> > > This is similarly confusing when using the chardev with a serial device,
> > > as reported in https://bugzilla.redhat.com/show_bug.cgi?id=2106975.
> > >
> > > It seems we have kept the support for bidirectional pipes for historical
> > > reasons and odd systems, however it's not documented in qemu -chardev
> > > options. I suggest to stop supporting it, for portability reasons.
> >
> > Hmm, I always assumed that in this scenario the pipe was operating
> > in output-only mode. Obviously not the case with the code as it
> > exists, but perhaps this would be useful ?  eg its good as a serial
> > console logging mechanism at least.
> 
> The current "-chardev pipe,id=id,path=path" option handling will first
> check the presence of unidirectional "path.in" & "path.out" (although
> they are opened RDWR...), and fallback on bidirectional "path".
> 
> We could allow for the presence of "path.out" alone, although this may
> be a behaviour/breaking change:

If we allow path.out alone, then we loose error diagnostic when
path.out is succesfully opened, but path.in fails. I wouldn't
call that a back compat breakage.

My preference would always be to use the exact path that was
given as the CLI parameter.

IOW, we really ought to have had

   -chardev pipe,id=id,input=path,output=path

and allowed both of them to be optional, eg both of them should
semantically mean /dev/null in behavioural terms if omitted

IOW we could just deprecate 'path' entirely and introduce this
saner approach to config.

Alternatively, I would just unconditionally change

diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 66d3b85091..3dda3d5cc6 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -142,7 +142,7 @@ static void qemu_chr_open_pipe(Chardev *chr,
         if (fd_out >= 0) {
             close(fd_out);
         }
-        TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
+        TFR(fd_in = fd_out = qemu_open_old(filename, O_WRONLY | O_BINARY));
         if (fd_in < 0) {
             error_setg_file_open(errp, errno, filename);
             return;


given that semantics on any UNIX platform we target are for pipes to be
unidirectional, and eating our own output is uselessly broken, we could
reasonably justify doing that change simply as a bug fix and ignore any
notion of 'deprecation',

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