From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

A win32 SOCKET handle is often cast to an int file descriptor, as this is what
other OS use for sockets. When necessary, QEMU eventually queries whether it's a
socket with the help of fd_is_socket(). However, there is no guarantee of
conflict between the fd and SOCKET space. Such conflict would have surprising
consequences. We can fix this by using FDs only.

After fixing a few missed closesocket(), this patch series makes the win32
socket API wrappers take FDs. It finally get rid of closesocket() usage by using
a close() wrapper instead. (note that fdopen/fclose would not be enough either
to close the underlying socket appropriately)

v3:
- fix closesocket() to prevent CloseHandle() from close()
- fix direct closesocket() usage (#undef closesocket before)
- add a test for &error_warn
- add r-b tags

v2:
- add clean up patch "util: drop qemu_fork()"
- add a "&error_warn", to help with basic error reporting
- fix errno handling after _get_osfhandle()
- introduce qemu_socket_(un)select() helpers
- add patch "aio_set_fd_handler() only supports SOCKET"
- add meson slirp.wrap RFC
- various misc cleanups
- add r-b tags

Marc-André Lureau (16):
  util: drop qemu_fork()
  tests: use closesocket()
  io: use closesocket()
  tests: add test-error-report
  error: add global &error_warn destination
  win32/socket: introduce qemu_socket_select() helper
  win32/socket: introduce qemu_socket_unselect() helper
  aio: make aio_set_fd_poll() static to aio-posix.c
  aio/win32: aio_set_fd_handler() only supports SOCKET
  RFC: build-sys: add slirp.wrap
  main-loop: remove qemu_fd_register(), win32/slirp/socket specific
  slirp: unregister the win32 SOCKET
  slirp: open-code qemu_socket_(un)select()
  win32: avoid mixing SOCKET and file descriptor space
  os-posix: remove useless ioctlsocket() define
  win32: replace closesocket() with close() wrapper

 include/block/aio.h            |   8 --
 include/qapi/error.h           |   6 +
 include/qemu/main-loop.h       |   2 -
 include/qemu/osdep.h           |  14 --
 include/sysemu/os-posix.h      |   3 -
 include/sysemu/os-win32.h      |  15 ++-
 backends/tpm/tpm_emulator.c    |   6 +-
 crypto/afalg.c                 |   6 +-
 hw/hyperv/syndbg.c             |   4 +-
 io/channel-socket.c            |   8 +-
 io/channel-watch.c             |  10 +-
 net/dgram.c                    |  14 +-
 net/slirp.c                    |  16 ++-
 net/socket.c                   |  22 +--
 tests/qtest/libqtest.c         |   8 +-
 tests/qtest/microbit-test.c    |   2 +-
 tests/qtest/netdev-socket.c    |  10 +-
 tests/unit/socket-helpers.c    |   2 +-
 tests/unit/test-error-report.c | 139 +++++++++++++++++++
 util/aio-posix.c               |   6 +-
 util/aio-win32.c               |  23 ++--
 util/error.c                   |  10 +-
 util/main-loop.c               |  11 --
 util/oslib-posix.c             |  70 ----------
 util/oslib-win32.c             | 240 ++++++++++++++++++++++++++++-----
 util/qemu-sockets.c            |  22 +--
 .gitignore                     |   2 +
 subprojects/slirp.wrap         |   6 +
 tests/unit/meson.build         |   1 +
 29 files changed, 461 insertions(+), 225 deletions(-)
 create mode 100644 tests/unit/test-error-report.c
 create mode 100644 subprojects/slirp.wrap

-- 
2.39.2

Hi

On Tue, Feb 21, 2023 at 4:48 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> A win32 SOCKET handle is often cast to an int file descriptor, as this is
> what
> other OS use for sockets. When necessary, QEMU eventually queries whether
> it's a
> socket with the help of fd_is_socket(). However, there is no guarantee of
> conflict between the fd and SOCKET space. Such conflict would have
> surprising
> consequences. We can fix this by using FDs only.
>
> After fixing a few missed closesocket(), this patch series makes the win32
> socket API wrappers take FDs. It finally get rid of closesocket() usage by
> using
> a close() wrapper instead. (note that fdopen/fclose would not be enough
> either
> to close the underlying socket appropriately)
>
> v3:
> - fix closesocket() to prevent CloseHandle() from close()
> - fix direct closesocket() usage (#undef closesocket before)
> - add a test for &error_warn
> - add r-b tags
>
>
ping  (I am missing reviews, thanks)


> v2:
> - add clean up patch "util: drop qemu_fork()"
> - add a "&error_warn", to help with basic error reporting
> - fix errno handling after _get_osfhandle()
> - introduce qemu_socket_(un)select() helpers
> - add patch "aio_set_fd_handler() only supports SOCKET"
> - add meson slirp.wrap RFC
> - various misc cleanups
> - add r-b tags
>
> Marc-André Lureau (16):
>   util: drop qemu_fork()
>   tests: use closesocket()
>   io: use closesocket()
>   tests: add test-error-report
>   error: add global &error_warn destination
>   win32/socket: introduce qemu_socket_select() helper
>   win32/socket: introduce qemu_socket_unselect() helper
>   aio: make aio_set_fd_poll() static to aio-posix.c
>   aio/win32: aio_set_fd_handler() only supports SOCKET
>   RFC: build-sys: add slirp.wrap
>   main-loop: remove qemu_fd_register(), win32/slirp/socket specific
>   slirp: unregister the win32 SOCKET
>   slirp: open-code qemu_socket_(un)select()
>   win32: avoid mixing SOCKET and file descriptor space
>   os-posix: remove useless ioctlsocket() define
>   win32: replace closesocket() with close() wrapper
>
>  include/block/aio.h            |   8 --
>  include/qapi/error.h           |   6 +
>  include/qemu/main-loop.h       |   2 -
>  include/qemu/osdep.h           |  14 --
>  include/sysemu/os-posix.h      |   3 -
>  include/sysemu/os-win32.h      |  15 ++-
>  backends/tpm/tpm_emulator.c    |   6 +-
>  crypto/afalg.c                 |   6 +-
>  hw/hyperv/syndbg.c             |   4 +-
>  io/channel-socket.c            |   8 +-
>  io/channel-watch.c             |  10 +-
>  net/dgram.c                    |  14 +-
>  net/slirp.c                    |  16 ++-
>  net/socket.c                   |  22 +--
>  tests/qtest/libqtest.c         |   8 +-
>  tests/qtest/microbit-test.c    |   2 +-
>  tests/qtest/netdev-socket.c    |  10 +-
>  tests/unit/socket-helpers.c    |   2 +-
>  tests/unit/test-error-report.c | 139 +++++++++++++++++++
>  util/aio-posix.c               |   6 +-
>  util/aio-win32.c               |  23 ++--
>  util/error.c                   |  10 +-
>  util/main-loop.c               |  11 --
>  util/oslib-posix.c             |  70 ----------
>  util/oslib-win32.c             | 240 ++++++++++++++++++++++++++++-----
>  util/qemu-sockets.c            |  22 +--
>  .gitignore                     |   2 +
>  subprojects/slirp.wrap         |   6 +
>  tests/unit/meson.build         |   1 +
>  29 files changed, 461 insertions(+), 225 deletions(-)
>  create mode 100644 tests/unit/test-error-report.c
>  create mode 100644 subprojects/slirp.wrap
>
> --
> 2.39.2
>
>

On 3/2/23 15:09, Marc-André Lureau wrote:
> 
> 
>     v3:
>     - fix closesocket() to prevent CloseHandle() from close()
>     - fix direct closesocket() usage (#undef closesocket before)
>     - add a test for &error_warn
>     - add r-b tags
> 
> ping  (I am missing reviews, thanks)

I'm going to queue this series today if that's fine for you---thanks for 
reminding!

Paolo

Hi Paolo

On Mon, Mar 6, 2023 at 12:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 3/2/23 15:09, Marc-André Lureau wrote:
> >
> >
> >     v3:
> >     - fix closesocket() to prevent CloseHandle() from close()
> >     - fix direct closesocket() usage (#undef closesocket before)
> >     - add a test for &error_warn
> >     - add r-b tags
> >
> > ping  (I am missing reviews, thanks)
>
> I'm going to queue this series today if that's fine for you---thanks for
> reminding!
>
>
Great, thanks! (I suppose you'll drop "RFC: build-sys: add slirp.wrap", and
perhaps queue the other meson/wrap series instead)

On 3/6/23 09:08, Marc-André Lureau wrote:
> Great, thanks! (I suppose you'll drop "RFC: build-sys: add slirp.wrap", 
> and perhaps queue the other meson/wrap series instead)

I don't have time to test the dtc fallback in CI, so I'll queue only the 
first three patches and note it as experimental in the release notes.

In the next release we'll probably have pip support in configure, so 
perhaps we can make the --wrap-mode option conditional.

Paolo