Re: [PATCH v3 16/16] win32: replace closesocket() with close() wrapper

[Stefan Berger]

On 2/21/23 07:48, marcandre.lur...@redhat.com wrote:

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

Use a close() wrapper instead, so that we don't need to worry about
closesocket() vs close() anymore, let's hope.

Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 7836fb0be3..29a667ae3d 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -370,39 +370,39 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr 
*addr,
  }


-#undef closesocket
-int qemu_closesocket_wrap(int fd)
+#undef close
+int qemu_close_wrap(int fd)
  {
      int ret;
      DWORD flags = 0;
-    SOCKET s = _get_osfhandle(fd);
+    SOCKET s = INVALID_SOCKET;

-    if (s == INVALID_SOCKET) {
-        return -1;
-    }
+    if (fd_is_socket(fd)) {
+        s = _get_osfhandle(fd);

-    /*
-     * If we were to just call _close on the descriptor, it would close the
-     * HANDLE, but it wouldn't free any of the resources associated to the
-     * SOCKET, and we can't call _close after calling closesocket, because
-     * closesocket has already closed the HANDLE, and _close would attempt to
-     * close the HANDLE again, resulting in a double free. We can however
-     * protect the HANDLE from actually being closed long enough to close the
-     * file descriptor, then close the socket itself.
-     */
-    if (!GetHandleInformation((HANDLE)s, &flags)) {
-        errno = EACCES;
-        return -1;
-    }
+        /*
+         * If we were to just call _close on the descriptor, it would close the
+         * HANDLE, but it wouldn't free any of the resources associated to the
+         * SOCKET, and we can't call _close after calling closesocket, because
+         * closesocket has already closed the HANDLE, and _close would attempt 
to
+         * close the HANDLE again, resulting in a double free. We can however
+         * protect the HANDLE from actually being closed long enough to close 
the
+         * file descriptor, then close the socket itself.
+         */
+        if (!GetHandleInformation((HANDLE)s, &flags)) {
+            errno = EACCES;
+            return -1;
+        }

-    if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, 
HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
-        errno = EACCES;
-        return -1;
+        if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, 
HANDLE_FLAG_PROTECT_FROM_CLOSE)) {
+            errno = EACCES;
+            return -1;
+        }
      }

      ret = close(fd);

-    if (!SetHandleInformation((HANDLE)s, flags, flags)) {
+    if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags, flags)) 
{
          errno = EACCES;
          return -1;
      }
@@ -411,13 +411,15 @@ int qemu_closesocket_wrap(int fd)
       * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying 
handle,
       * but the FD is actually freed
       */
-    if (ret < 0 && errno != EBADF) {
+    if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) {
          return ret;
      }

-    ret = closesocket(s);
-    if (ret < 0) {
-        errno = socket_error();
+    if (s != INVALID_SOCKET) {
+        ret = closesocket(s);
+        if (ret < 0) {
+            errno = socket_error();
+        }
      }

if (fs_is_socket()) {{
    ...
    close()
    ...
    closesocket()
    ...
} else {
    ...
    close()
    ...
}

would avoid the threading and make this function look a bit simpler.

Reviewed-by: Stefan Berger <stef...@linux.ibm.com>