* [PATCH] qio: fix command spawn RDONLY/WRONLY @ 2022-09-01 10:11 marcandre.lureau 2022-09-01 10:31 ` Daniel P. Berrangé 0 siblings, 1 reply; 4+ messages in thread From: marcandre.lureau @ 2022-09-01 10:11 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, Marc-André Lureau From: Marc-André Lureau <marcandre.lureau@redhat.com> The in/out handling is inverted, although nothing seemed to notice that yet. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- io/channel-command.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/io/channel-command.c b/io/channel-command.c index 9f2f4a1793..ed17b44f74 100644 --- a/io/channel-command.c +++ b/io/channel-command.c @@ -79,10 +79,10 @@ qio_channel_command_new_spawn(const char *const argv[], flags = flags & O_ACCMODE; if (flags == O_RDONLY) { - stdinnull = true; + stdoutnull = true; } if (flags == O_WRONLY) { - stdoutnull = true; + stdinnull = true; } if (stdinnull || stdoutnull) { -- 2.37.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] qio: fix command spawn RDONLY/WRONLY 2022-09-01 10:11 [PATCH] qio: fix command spawn RDONLY/WRONLY marcandre.lureau @ 2022-09-01 10:31 ` Daniel P. Berrangé 2022-09-01 10:47 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Daniel P. Berrangé @ 2022-09-01 10:31 UTC (permalink / raw) To: marcandre.lureau; +Cc: qemu-devel On Thu, Sep 01, 2022 at 02:11:20PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > The in/out handling is inverted, although nothing seemed to notice that yet. On the contrary, it is correct, and the unit tests validate this. > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > io/channel-command.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/io/channel-command.c b/io/channel-command.c > index 9f2f4a1793..ed17b44f74 100644 > --- a/io/channel-command.c > +++ b/io/channel-command.c > @@ -79,10 +79,10 @@ qio_channel_command_new_spawn(const char *const argv[], > flags = flags & O_ACCMODE; > > if (flags == O_RDONLY) { > - stdinnull = true; > + stdoutnull = true; > } > if (flags == O_WRONLY) { > - stdoutnull = true; > + stdinnull = true; > } This change breaks the unit tests. The confusion is because there are two parties involves. The 'flags' variable is from the POV of the parent process, while stdinnull/stdoutnull are from the POV of the child process. IOW, if the parent process is reading from the child (O_RDONLY), then the child needs a stdout to write to the parent, but not any stdin to read from the parent, hence we set stdin to /dev/null in the child. 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 :| ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qio: fix command spawn RDONLY/WRONLY 2022-09-01 10:31 ` Daniel P. Berrangé @ 2022-09-01 10:47 ` Marc-André Lureau 2022-09-01 10:51 ` Daniel P. Berrangé 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2022-09-01 10:47 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 1754 bytes --] Hi On Thu, Sep 1, 2022 at 2:32 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Sep 01, 2022 at 02:11:20PM +0400, marcandre.lureau@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > The in/out handling is inverted, although nothing seemed to notice that > yet. > > On the contrary, it is correct, and the unit tests validate this. > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > io/channel-command.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/io/channel-command.c b/io/channel-command.c > > index 9f2f4a1793..ed17b44f74 100644 > > --- a/io/channel-command.c > > +++ b/io/channel-command.c > > @@ -79,10 +79,10 @@ qio_channel_command_new_spawn(const char *const > argv[], > > flags = flags & O_ACCMODE; > > > > if (flags == O_RDONLY) { > > - stdinnull = true; > > + stdoutnull = true; > > } > > if (flags == O_WRONLY) { > > - stdoutnull = true; > > + stdinnull = true; > > } > > This change breaks the unit tests. > > Does it really test it then? we are talking about test-io-channel-command ? It works before and after for me. Other tests as well. The confusion is because there are two parties involves. The 'flags' > variable is from the POV of the parent process, while stdinnull/stdoutnull > are from the POV of the child process. > > IOW, if the parent process is reading from the child (O_RDONLY), > then the child needs a stdout to write to the parent, but not > any stdin to read from the parent, hence we set stdin to /dev/null > in the child. > Ok, thanks for the clarification! -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 2843 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qio: fix command spawn RDONLY/WRONLY 2022-09-01 10:47 ` Marc-André Lureau @ 2022-09-01 10:51 ` Daniel P. Berrangé 0 siblings, 0 replies; 4+ messages in thread From: Daniel P. Berrangé @ 2022-09-01 10:51 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel On Thu, Sep 01, 2022 at 02:47:17PM +0400, Marc-André Lureau wrote: > Hi > > On Thu, Sep 1, 2022 at 2:32 PM Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > On Thu, Sep 01, 2022 at 02:11:20PM +0400, marcandre.lureau@redhat.com > > wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > The in/out handling is inverted, although nothing seemed to notice that > > yet. > > > > On the contrary, it is correct, and the unit tests validate this. > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > io/channel-command.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/io/channel-command.c b/io/channel-command.c > > > index 9f2f4a1793..ed17b44f74 100644 > > > --- a/io/channel-command.c > > > +++ b/io/channel-command.c > > > @@ -79,10 +79,10 @@ qio_channel_command_new_spawn(const char *const > > argv[], > > > flags = flags & O_ACCMODE; > > > > > > if (flags == O_RDONLY) { > > > - stdinnull = true; > > > + stdoutnull = true; > > > } > > > if (flags == O_WRONLY) { > > > - stdoutnull = true; > > > + stdinnull = true; > > > } > > > > This change breaks the unit tests. > > > > > Does it really test it then? we are talking about test-io-channel-command ? > It works before and after for me. Other tests as well. Yes, it certainly fails for me: $ ./build/tests/unit/test-io-channel-command -p /io/channel/command/fifo/sync # random seed: R02S5134a426f643aeefdbe73921046b66da # Start of io tests # Start of channel tests # Start of command tests # Start of fifo tests ** ERROR:../tests/unit/io-channel-helpers.c:152:qio_channel_test_validate: assertion failed: (test->readerr == NULL) Bail out! ERROR:../tests/unit/io-channel-helpers.c:152:qio_channel_test_validate: assertion failed: (test->readerr == NULL) Aborted (core dumped) 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 :| ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-01 10:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-01 10:11 [PATCH] qio: fix command spawn RDONLY/WRONLY marcandre.lureau 2022-09-01 10:31 ` Daniel P. Berrangé 2022-09-01 10:47 ` Marc-André Lureau 2022-09-01 10:51 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).