1. Patchew
  2. QEMU
  3. [PATCH 00/51] tests/qtest: Enable running qtest on Windows
  4. View patch

[PATCH 27/51] tests/qtest: Use send/recv for socket communication

Bin Meng posted 51 patches 3 years, 5 months ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Thomas Huth <thuth@redhat.com>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Berger <stefanb@linux.vnet.ibm.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Greg Kurz <groug@kaod.org>, Christian Schoenebeck <qemu_oss@crudebyte.com>, "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Gerd Hoffmann <kraxel@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, John Snow <jsnow@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>, Havard Skinnemoen <hskinnemoen@google.com>, Tyrone Ting <kfting@nuvoton.com>, Markus Armbruster <armbru@redhat.com>, Coiby Xu <Coiby.Xu@gmail.com>, Jason Wang <jasowang@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Bin Meng 3 years, 5 months ago 
From: Xuzhou Cheng <xuzhou.cheng@windriver.com>

Socket communication in the libqtest and libqmp codes uses read()
and write() which work on any file descriptor on *nix, and sockets
in *nix are an example of a file descriptor.

However sockets on Windows do not use *nix-style file descriptors,
so read() and write() cannot be used on sockets on Windows.
Switch over to use send() and recv() instead which work on both
Windows and *nix.

Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 tests/qtest/libqmp.c   | 4 ++--
 tests/qtest/libqtest.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
index ade26c15f0..995a39c1f8 100644
--- a/tests/qtest/libqmp.c
+++ b/tests/qtest/libqmp.c
@@ -36,7 +36,7 @@ typedef struct {
 
 static void socket_send(int fd, const char *buf, size_t size)
 {
-    size_t res = qemu_write_full(fd, buf, size);
+    ssize_t res = send(fd, buf, size, 0);
 
     assert(res == size);
 }
@@ -69,7 +69,7 @@ QDict *qmp_fd_receive(int fd)
         ssize_t len;
         char c;
 
-        len = read(fd, &c, 1);
+        len = recv(fd, &c, 1, 0);
         if (len == -1 && errno == EINTR) {
             continue;
         }
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 909583dad3..b7b7c9c541 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -438,7 +438,7 @@ void qtest_quit(QTestState *s)
 
 static void socket_send(int fd, const char *buf, size_t size)
 {
-    size_t res = qemu_write_full(fd, buf, size);
+    ssize_t res = send(fd, buf, size, 0);
 
     assert(res == size);
 }
@@ -470,7 +470,7 @@ static GString *qtest_client_socket_recv_line(QTestState *s)
         ssize_t len;
         char buffer[1024];
 
-        len = read(s->fd, buffer, sizeof(buffer));
+        len = recv(s->fd, buffer, sizeof(buffer), 0);
         if (len == -1 && errno == EINTR) {
             continue;
         }
-- 
2.34.1
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Thomas Huth 3 years, 5 months ago 
On 24/08/2022 11.40, Bin Meng wrote:
> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> 
> Socket communication in the libqtest and libqmp codes uses read()
> and write() which work on any file descriptor on *nix, and sockets
> in *nix are an example of a file descriptor.
> 
> However sockets on Windows do not use *nix-style file descriptors,
> so read() and write() cannot be used on sockets on Windows.
> Switch over to use send() and recv() instead which work on both
> Windows and *nix.
> 
> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   tests/qtest/libqmp.c   | 4 ++--
>   tests/qtest/libqtest.c | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
> index ade26c15f0..995a39c1f8 100644
> --- a/tests/qtest/libqmp.c
> +++ b/tests/qtest/libqmp.c
> @@ -36,7 +36,7 @@ typedef struct {
>   
>   static void socket_send(int fd, const char *buf, size_t size)
>   {
> -    size_t res = qemu_write_full(fd, buf, size);
> +    ssize_t res = send(fd, buf, size, 0);

This way we're losing the extra logic from qemu_write_full() here (i.e. the 
looping and EINTR handling) ... not sure whether that's really OK? Maybe you 
have to introduce a qemu_send_full() first?

  Thomas
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Bin Meng 3 years, 5 months ago 
On Thu, Aug 25, 2022 at 9:04 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 24/08/2022 11.40, Bin Meng wrote:
> > From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> >
> > Socket communication in the libqtest and libqmp codes uses read()
> > and write() which work on any file descriptor on *nix, and sockets
> > in *nix are an example of a file descriptor.
> >
> > However sockets on Windows do not use *nix-style file descriptors,
> > so read() and write() cannot be used on sockets on Windows.
> > Switch over to use send() and recv() instead which work on both
> > Windows and *nix.
> >
> > Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >   tests/qtest/libqmp.c   | 4 ++--
> >   tests/qtest/libqtest.c | 4 ++--
> >   2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
> > index ade26c15f0..995a39c1f8 100644
> > --- a/tests/qtest/libqmp.c
> > +++ b/tests/qtest/libqmp.c
> > @@ -36,7 +36,7 @@ typedef struct {
> >
> >   static void socket_send(int fd, const char *buf, size_t size)
> >   {
> > -    size_t res = qemu_write_full(fd, buf, size);
> > +    ssize_t res = send(fd, buf, size, 0);
>
> This way we're losing the extra logic from qemu_write_full() here (i.e. the
> looping and EINTR handling) ... not sure whether that's really OK? Maybe you
> have to introduce a qemu_send_full() first?
>

I am not sure if qemu_send_full() is really needed because there is an
assert() right after the send() call.

Regards,
Bin
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Thomas Huth 3 years, 5 months ago 
On 26/08/2022 16.59, Bin Meng wrote:
> On Thu, Aug 25, 2022 at 9:04 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 24/08/2022 11.40, Bin Meng wrote:
>>> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>>>
>>> Socket communication in the libqtest and libqmp codes uses read()
>>> and write() which work on any file descriptor on *nix, and sockets
>>> in *nix are an example of a file descriptor.
>>>
>>> However sockets on Windows do not use *nix-style file descriptors,
>>> so read() and write() cannot be used on sockets on Windows.
>>> Switch over to use send() and recv() instead which work on both
>>> Windows and *nix.
>>>
>>> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>> ---
>>>
>>>    tests/qtest/libqmp.c   | 4 ++--
>>>    tests/qtest/libqtest.c | 4 ++--
>>>    2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
>>> index ade26c15f0..995a39c1f8 100644
>>> --- a/tests/qtest/libqmp.c
>>> +++ b/tests/qtest/libqmp.c
>>> @@ -36,7 +36,7 @@ typedef struct {
>>>
>>>    static void socket_send(int fd, const char *buf, size_t size)
>>>    {
>>> -    size_t res = qemu_write_full(fd, buf, size);
>>> +    ssize_t res = send(fd, buf, size, 0);
>>
>> This way we're losing the extra logic from qemu_write_full() here (i.e. the
>> looping and EINTR handling) ... not sure whether that's really OK? Maybe you
>> have to introduce a qemu_send_full() first?
>>
> 
> I am not sure if qemu_send_full() is really needed because there is an
> assert() right after the send() call.

That's just a sanity check ... I think this function still has to take care 
of EINTR - it originally looked like this:

  https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c3e5704af19ac6

... and you can also see the while loop there.

  Thomas
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Marc-André Lureau 3 years, 5 months ago 
Hi

On Fri, Aug 26, 2022 at 10:27 PM Thomas Huth <thuth@redhat.com> wrote:

> On 26/08/2022 16.59, Bin Meng wrote:
> > On Thu, Aug 25, 2022 at 9:04 PM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 24/08/2022 11.40, Bin Meng wrote:
> >>> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> >>>
> >>> Socket communication in the libqtest and libqmp codes uses read()
> >>> and write() which work on any file descriptor on *nix, and sockets
> >>> in *nix are an example of a file descriptor.
> >>>
> >>> However sockets on Windows do not use *nix-style file descriptors,
> >>> so read() and write() cannot be used on sockets on Windows.
> >>> Switch over to use send() and recv() instead which work on both
> >>> Windows and *nix.
> >>>
> >>> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >>> ---
> >>>
> >>>    tests/qtest/libqmp.c   | 4 ++--
> >>>    tests/qtest/libqtest.c | 4 ++--
> >>>    2 files changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
> >>> index ade26c15f0..995a39c1f8 100644
> >>> --- a/tests/qtest/libqmp.c
> >>> +++ b/tests/qtest/libqmp.c
> >>> @@ -36,7 +36,7 @@ typedef struct {
> >>>
> >>>    static void socket_send(int fd, const char *buf, size_t size)
> >>>    {
> >>> -    size_t res = qemu_write_full(fd, buf, size);
> >>> +    ssize_t res = send(fd, buf, size, 0);
> >>
> >> This way we're losing the extra logic from qemu_write_full() here (i.e.
> the
> >> looping and EINTR handling) ... not sure whether that's really OK?
> Maybe you
> >> have to introduce a qemu_send_full() first?
> >>
> >
> > I am not sure if qemu_send_full() is really needed because there is an
> > assert() right after the send() call.
>
> That's just a sanity check ... I think this function still has to take
> care
> of EINTR - it originally looked like this:
>
>   https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c3e5704af19ac6
>
> ... and you can also see the while loop there.
>
>
Agree, that would be the correct thing to do.

Fwiw, the SOCKET vs fd situation is giving me some nervous feelings,
sometimes.

For ex, as I checked recently, it seems close(fd) correctly closes the
underlying SOCKET - as if closesocket() was called on it.. but this is not
really documented.

And it's easy to mix fd vs SOCKET in QEMU code paths (we cast/map SOCKET to
"int fd" in general), and reach a close() on a SOCKET. That wouldn't be
valid, and would likely create leaks or other issues.

Maybe we should introduce a type for safety / documentation purposes...

-- 
Marc-André Lureau
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Bin Meng 3 years, 5 months ago 
On Wed, Aug 31, 2022 at 10:06 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Aug 26, 2022 at 10:27 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 26/08/2022 16.59, Bin Meng wrote:
>> > On Thu, Aug 25, 2022 at 9:04 PM Thomas Huth <thuth@redhat.com> wrote:
>> >>
>> >> On 24/08/2022 11.40, Bin Meng wrote:
>> >>> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>> >>>
>> >>> Socket communication in the libqtest and libqmp codes uses read()
>> >>> and write() which work on any file descriptor on *nix, and sockets
>> >>> in *nix are an example of a file descriptor.
>> >>>
>> >>> However sockets on Windows do not use *nix-style file descriptors,
>> >>> so read() and write() cannot be used on sockets on Windows.
>> >>> Switch over to use send() and recv() instead which work on both
>> >>> Windows and *nix.
>> >>>
>> >>> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>> >>> ---
>> >>>
>> >>>    tests/qtest/libqmp.c   | 4 ++--
>> >>>    tests/qtest/libqtest.c | 4 ++--
>> >>>    2 files changed, 4 insertions(+), 4 deletions(-)
>> >>>
>> >>> diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
>> >>> index ade26c15f0..995a39c1f8 100644
>> >>> --- a/tests/qtest/libqmp.c
>> >>> +++ b/tests/qtest/libqmp.c
>> >>> @@ -36,7 +36,7 @@ typedef struct {
>> >>>
>> >>>    static void socket_send(int fd, const char *buf, size_t size)
>> >>>    {
>> >>> -    size_t res = qemu_write_full(fd, buf, size);
>> >>> +    ssize_t res = send(fd, buf, size, 0);
>> >>
>> >> This way we're losing the extra logic from qemu_write_full() here (i.e. the
>> >> looping and EINTR handling) ... not sure whether that's really OK? Maybe you
>> >> have to introduce a qemu_send_full() first?
>> >>
>> >
>> > I am not sure if qemu_send_full() is really needed because there is an
>> > assert() right after the send() call.
>>
>> That's just a sanity check ... I think this function still has to take care
>> of EINTR - it originally looked like this:
>>
>>   https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c3e5704af19ac6
>>
>> ... and you can also see the while loop there.
>>
>
> Agree, that would be the correct thing to do.
>
> Fwiw, the SOCKET vs fd situation is giving me some nervous feelings, sometimes.
>
> For ex, as I checked recently, it seems close(fd) correctly closes the underlying SOCKET - as if closesocket() was called on it.. but this is not really documented.

Really? If you use gdb to step over close(socket) on Windows, you will
see a Windows debug message is thrown to gdb saying that:

"warning: Invalid parameter passed to C runtime function."

MSDN only says closesocket() should be used on socket. This is why in
the QEMU codes we map closesocket to close on POSIX, and always use
closesocket.

>
> And it's easy to mix fd vs SOCKET in QEMU code paths (we cast/map SOCKET to "int fd" in general), and reach a close() on a SOCKET. That wouldn't be valid, and would likely create leaks or other issues.
>
> Maybe we should introduce a type for safety / documentation purposes...
>

Regards,
Bin
Re: [PATCH 27/51] tests/qtest: Use send/recv for socket communication
Posted by Daniel P. Berrangé 3 years, 5 months ago 
On Wed, Aug 31, 2022 at 06:05:51PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Aug 26, 2022 at 10:27 PM Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 26/08/2022 16.59, Bin Meng wrote:
> > > On Thu, Aug 25, 2022 at 9:04 PM Thomas Huth <thuth@redhat.com> wrote:
> > >>
> > >> On 24/08/2022 11.40, Bin Meng wrote:
> > >>> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> > >>>
> > >>> Socket communication in the libqtest and libqmp codes uses read()
> > >>> and write() which work on any file descriptor on *nix, and sockets
> > >>> in *nix are an example of a file descriptor.
> > >>>
> > >>> However sockets on Windows do not use *nix-style file descriptors,
> > >>> so read() and write() cannot be used on sockets on Windows.
> > >>> Switch over to use send() and recv() instead which work on both
> > >>> Windows and *nix.
> > >>>
> > >>> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> > >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > >>> ---
> > >>>
> > >>>    tests/qtest/libqmp.c   | 4 ++--
> > >>>    tests/qtest/libqtest.c | 4 ++--
> > >>>    2 files changed, 4 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
> > >>> index ade26c15f0..995a39c1f8 100644
> > >>> --- a/tests/qtest/libqmp.c
> > >>> +++ b/tests/qtest/libqmp.c
> > >>> @@ -36,7 +36,7 @@ typedef struct {
> > >>>
> > >>>    static void socket_send(int fd, const char *buf, size_t size)
> > >>>    {
> > >>> -    size_t res = qemu_write_full(fd, buf, size);
> > >>> +    ssize_t res = send(fd, buf, size, 0);
> > >>
> > >> This way we're losing the extra logic from qemu_write_full() here (i.e.
> > the
> > >> looping and EINTR handling) ... not sure whether that's really OK?
> > Maybe you
> > >> have to introduce a qemu_send_full() first?
> > >>
> > >
> > > I am not sure if qemu_send_full() is really needed because there is an
> > > assert() right after the send() call.
> >
> > That's just a sanity check ... I think this function still has to take
> > care
> > of EINTR - it originally looked like this:
> >
> >   https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c3e5704af19ac6
> >
> > ... and you can also see the while loop there.
> >
> >
> Agree, that would be the correct thing to do.
> 
> Fwiw, the SOCKET vs fd situation is giving me some nervous feelings,
> sometimes.
> 
> For ex, as I checked recently, it seems close(fd) correctly closes the
> underlying SOCKET - as if closesocket() was called on it.. but this is not
> really documented.
> 
> And it's easy to mix fd vs SOCKET in QEMU code paths (we cast/map SOCKET to
> "int fd" in general), and reach a close() on a SOCKET. That wouldn't be
> valid, and would likely create leaks or other issues.
> 
> Maybe we should introduce a type for safety / documentation purposes...

We already have QIOChannel APIs, the problem here is that libtest is still
using low level sockets APIs and needs converting to the high level APIs
instead.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.