Re: [PATCH] RFC: char: deprecate usage of bidirectional pipe
On Tue, Jul 26, 2022 at 09:44:25AM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 26, 2022 at 12:32:32PM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > 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.
Well, using ${file}.in and ${file}.out has the advantage that it works
fine with all qemu versions. So adding a warning suggesting to use that
makes sense to me, especially as 7.1 fix.
When looking at longer-term improvements it is probably better to add
support for explicit in/out pipe names, i.e. input= and output=
parameters as suggested later in the thread. Avoids needing to know
qemu internals (pipe naming convention) and allows to make the input
pipe optional for the logging use case. That probably is something we
don't want rush into 7.1 though ...
take care,
Gerd
Re: [PATCH] RFC: char: deprecate usage of bidirectional pipe
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é
> wrote:
> >
> > On Tue, Jul 26, 2022 at 12:32:32PM +0400, marcandre.lur...@redhat.com wrote:
> > > From: Marc-André Lureau
> > >
> > > 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 :|
Re: [PATCH] RFC: char: deprecate usage of bidirectional pipe
Hi
On Tue, Jul 26, 2022 at 12:44 PM Daniel P. Berrangé wrote:
>
> On Tue, Jul 26, 2022 at 12:32:32PM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > 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:
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 7db963035e..f78bcd7daf 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -137,12 +137,12 @@ static void qemu_chr_open_pipe(Chardev *chr,
g_free(filename_in);
g_free(filename_out);
if (fd_in < 0 || fd_out < 0) {
+if (fd_out >= 0) {
+goto out;
+}
if (fd_in >= 0) {
close(fd_in);
}
-if (fd_out >= 0) {
-close(fd_out);
-}
warn_report("Support for bidirectional pipe is deprecated,");
warn_report("please use portable one-way pipes instead (%s.in
& %s.out).",
filename, filename);
@@ -152,6 +152,7 @@ static void qemu_chr_open_pipe(Chardev *chr,
return;
}
}
+out:
qemu_chr_open_fd(chr, fd_in, fd_out);
or we introduce a new "access=write" option, or a new chardev type ?
>
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > docs/about/deprecated.rst | 6 ++
> > chardev/char-pipe.c | 4
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 7ee26626d5cf..dd5ca30d527b 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -45,6 +45,12 @@ transmit audio through the VNC protocol.
> > ``tty`` and ``parport`` are aliases that will be removed. Instead, the
> > actual backend names ``serial`` and ``parallel`` should be used.
> >
> > +``-chardev pipe`` support for bidirectional pipes (since 7.1)
> > +
> > +
> > +For portability reasons, the support for bidirectional ``pipe`` will
> > +be removed. Instead, use ``file.in`` & ``file.out`` unidirectional pipes.
> > +
> > Short-form boolean options (since 6.0)
> > ''
> >
> > diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> > index 66d3b8509183..7db963035e7d 100644
> > --- a/chardev/char-pipe.c
> > +++ b/chardev/char-pipe.c
> > @@ -27,6 +27,7 @@
> > #include "qemu/main-loop.h"
> > #include "qemu/module.h"
> > #include "qemu/option.h"
> > +#include "qemu/error-report.h"
> > #include "chardev/char.h"
> >
> > #ifdef _WIN32
> > @@ -142,6 +143,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
> > if (fd_out >= 0) {
> > close(fd_out);
> > }
> > +warn_report("Support for bidirectional pipe is deprecated,");
> > +warn_report("please use portable one-way pipes instead (%s.in &
> > %s.out).",
> > +filename, filename);
> > TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> > if (fd_in < 0) {
> > error_setg_file_open(errp, errno, filename);
> > --
> > 2.37.0.rc0
> >
>
> 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 :|
>
Re: [PATCH] RFC: char: deprecate usage of bidirectional pipe
On Tue, Jul 26, 2022 at 12:32:32PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> 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.
>
> Signed-off-by: Marc-André Lureau
> ---
> docs/about/deprecated.rst | 6 ++
> chardev/char-pipe.c | 4
> 2 files changed, 10 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 7ee26626d5cf..dd5ca30d527b 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -45,6 +45,12 @@ transmit audio through the VNC protocol.
> ``tty`` and ``parport`` are aliases that will be removed. Instead, the
> actual backend names ``serial`` and ``parallel`` should be used.
>
> +``-chardev pipe`` support for bidirectional pipes (since 7.1)
> +
> +
> +For portability reasons, the support for bidirectional ``pipe`` will
> +be removed. Instead, use ``file.in`` & ``file.out`` unidirectional pipes.
> +
> Short-form boolean options (since 6.0)
> ''
>
> diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
> index 66d3b8509183..7db963035e7d 100644
> --- a/chardev/char-pipe.c
> +++ b/chardev/char-pipe.c
> @@ -27,6 +27,7 @@
> #include "qemu/main-loop.h"
> #include "qemu/module.h"
> #include "qemu/option.h"
> +#include "qemu/error-report.h"
> #include "chardev/char.h"
>
> #ifdef _WIN32
> @@ -142,6 +143,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
> if (fd_out >= 0) {
> close(fd_out);
> }
> +warn_report("Support for bidirectional pipe is deprecated,");
> +warn_report("please use portable one-way pipes instead (%s.in &
> %s.out).",
> +filename, filename);
> TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
> if (fd_in < 0) {
> error_setg_file_open(errp, errno, filename);
> --
> 2.37.0.rc0
>
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 :|
[PATCH] RFC: char: deprecate usage of bidirectional pipe
From: Marc-André Lureau
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.
Signed-off-by: Marc-André Lureau
---
docs/about/deprecated.rst | 6 ++
chardev/char-pipe.c | 4
2 files changed, 10 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7ee26626d5cf..dd5ca30d527b 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -45,6 +45,12 @@ transmit audio through the VNC protocol.
``tty`` and ``parport`` are aliases that will be removed. Instead, the
actual backend names ``serial`` and ``parallel`` should be used.
+``-chardev pipe`` support for bidirectional pipes (since 7.1)
+
+
+For portability reasons, the support for bidirectional ``pipe`` will
+be removed. Instead, use ``file.in`` & ``file.out`` unidirectional pipes.
+
Short-form boolean options (since 6.0)
''
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 66d3b8509183..7db963035e7d 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -27,6 +27,7 @@
#include "qemu/main-loop.h"
#include "qemu/module.h"
#include "qemu/option.h"
+#include "qemu/error-report.h"
#include "chardev/char.h"
#ifdef _WIN32
@@ -142,6 +143,9 @@ static void qemu_chr_open_pipe(Chardev *chr,
if (fd_out >= 0) {
close(fd_out);
}
+warn_report("Support for bidirectional pipe is deprecated,");
+warn_report("please use portable one-way pipes instead (%s.in &
%s.out).",
+filename, filename);
TFR(fd_in = fd_out = qemu_open_old(filename, O_RDWR | O_BINARY));
if (fd_in < 0) {
error_setg_file_open(errp, errno, filename);
--
2.37.0.rc0
