| Message ID | 20220824094029.1634519-28-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/qtest: Enable running qtest on Windows | expand |
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
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
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
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...
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
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
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; }