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

[Bin Meng]

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