>From 36be0df632e8bc5ad7ccfe2745ed3254c95642d1 Mon Sep 17 00:00:00 2001 From: Bruno Haible Date: Sat, 10 Sep 2022 02:30:14 +0200 Subject: [PATCH 4/5] posix_spawn-internal: Refactor. * lib/windows-spawn.h (struct IHANDLE): New type. (struct inheritable_handles): Combine handles and flags into a single array. * lib/windows-spawn.c (init_inheritable_handles, compose_handles_block, spawnpvech): Update. * lib/spawni.c (grow_inheritable_handles, shrink_inheritable_handles, do_open, do_dup2, do_close): Update. --- ChangeLog | 9 ++++++ lib/spawni.c | 78 +++++++++++++++++++-------------------------- lib/windows-spawn.c | 71 +++++++++++++++++------------------------ lib/windows-spawn.h | 35 +++++++++++--------- 4 files changed, 92 insertions(+), 101 deletions(-) diff --git a/ChangeLog b/ChangeLog index 93ec3d78f2..0a11fb895c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ 2022-09-09 Bruno Haible + posix_spawn-internal: Refactor. + * lib/windows-spawn.h (struct IHANDLE): New type. + (struct inheritable_handles): Combine handles and flags into a single + array. + * lib/windows-spawn.c (init_inheritable_handles, compose_handles_block, + spawnpvech): Update. + * lib/spawni.c (grow_inheritable_handles, shrink_inheritable_handles, + do_open, do_dup2, do_close): Update. + posix_spawn-internal: Optimize DuplicateHandle calls on native Windows. * lib/spawni.c (open_handle): Return an inheritable HANDLE. (do_open): Don't call DuplicateHandle. Remove curr_process parameter. diff --git a/lib/spawni.c b/lib/spawni.c index 1850c7ddd5..ca99c5bff1 100644 --- a/lib/spawni.c +++ b/lib/spawni.c @@ -121,32 +121,22 @@ grow_inheritable_handles (struct inheritable_handles *inh_handles, int newfd) size_t new_allocated = 2 * inh_handles->allocated + 1; if (new_allocated <= newfd) new_allocated = newfd + 1; - HANDLE *new_handles_array = - (HANDLE *) - realloc (inh_handles->handles, new_allocated * sizeof (HANDLE)); - if (new_handles_array == NULL) + struct IHANDLE *new_ih = + (struct IHANDLE *) + realloc (inh_handles->ih, new_allocated * sizeof (struct IHANDLE)); + if (new_ih == NULL) { errno = ENOMEM; return -1; } - unsigned short *new_flags_array = - (unsigned short *) - realloc (inh_handles->flags, new_allocated * sizeof (unsigned short)); - if (new_flags_array == NULL) - { - free (new_handles_array); - errno = ENOMEM; - return -1; - } inh_handles->allocated = new_allocated; - inh_handles->handles = new_handles_array; - inh_handles->flags = new_flags_array; + inh_handles->ih = new_ih; } - HANDLE *handles = inh_handles->handles; + struct IHANDLE *ih = inh_handles->ih; for (; inh_handles->count <= newfd; inh_handles->count++) - handles[inh_handles->count] = INVALID_HANDLE_VALUE; + ih[inh_handles->count].handle = INVALID_HANDLE_VALUE; return 0; } @@ -156,26 +146,25 @@ grow_inheritable_handles (struct inheritable_handles *inh_handles, int newfd) static void shrink_inheritable_handles (struct inheritable_handles *inh_handles) { - HANDLE *handles = inh_handles->handles; - unsigned short *flags = inh_handles->flags; + struct IHANDLE *ih = inh_handles->ih; size_t handles_count = inh_handles->count; unsigned int fd; for (fd = 0; fd < handles_count; fd++) { - HANDLE handle = handles[fd]; + HANDLE handle = ih[fd].handle; if (handle != INVALID_HANDLE_VALUE - && (flags[fd] & KEEP_OPEN_IN_CHILD) == 0) + && (ih[fd].flags & KEEP_OPEN_IN_CHILD) == 0) { - if (!(flags[fd] & KEEP_OPEN_IN_PARENT)) + if (!(ih[fd].flags & KEEP_OPEN_IN_PARENT)) CloseHandle (handle); - handles[fd] = INVALID_HANDLE_VALUE; + ih[fd].handle = INVALID_HANDLE_VALUE; } } while (handles_count > 3 - && handles[handles_count - 1] == INVALID_HANDLE_VALUE) + && ih[handles_count - 1].handle == INVALID_HANDLE_VALUE) handles_count--; inh_handles->count = handles_count; @@ -185,17 +174,16 @@ shrink_inheritable_handles (struct inheritable_handles *inh_handles) static void close_inheritable_handles (struct inheritable_handles *inh_handles) { - HANDLE *handles = inh_handles->handles; - unsigned short *flags = inh_handles->flags; + struct IHANDLE *ih = inh_handles->ih; size_t handles_count = inh_handles->count; unsigned int fd; for (fd = 0; fd < handles_count; fd++) { - HANDLE handle = handles[fd]; + HANDLE handle = ih[fd].handle; if (handle != INVALID_HANDLE_VALUE - && !(flags[fd] & KEEP_OPEN_IN_PARENT)) + && !(ih[fd].flags & KEEP_OPEN_IN_PARENT)) CloseHandle (handle); } } @@ -397,9 +385,9 @@ do_open (struct inheritable_handles *inh_handles, int newfd, } if (grow_inheritable_handles (inh_handles, newfd) < 0) return -1; - if (inh_handles->handles[newfd] != INVALID_HANDLE_VALUE - && !(inh_handles->flags[newfd] & KEEP_OPEN_IN_PARENT) - && !CloseHandle (inh_handles->handles[newfd])) + if (inh_handles->ih[newfd].handle != INVALID_HANDLE_VALUE + && !(inh_handles->ih[newfd].flags & KEEP_OPEN_IN_PARENT) + && !CloseHandle (inh_handles->ih[newfd].handle)) { errno = EIO; return -1; @@ -428,8 +416,8 @@ do_open (struct inheritable_handles *inh_handles, int newfd, return -1; } free (filename_to_free); - inh_handles->handles[newfd] = handle; - inh_handles->flags[newfd] = + inh_handles->ih[newfd].handle = handle; + inh_handles->ih[newfd].flags = ((flags & O_APPEND) != 0 ? 32 : 0) | KEEP_OPEN_IN_CHILD; return 0; } @@ -442,7 +430,7 @@ do_dup2 (struct inheritable_handles *inh_handles, int oldfd, int newfd, HANDLE curr_process) { if (!(oldfd >= 0 && oldfd < inh_handles->count - && inh_handles->handles[oldfd] != INVALID_HANDLE_VALUE)) + && inh_handles->ih[oldfd].handle != INVALID_HANDLE_VALUE)) { errno = EBADF; return -1; @@ -456,24 +444,24 @@ do_dup2 (struct inheritable_handles *inh_handles, int oldfd, int newfd, { if (grow_inheritable_handles (inh_handles, newfd) < 0) return -1; - if (inh_handles->handles[newfd] != INVALID_HANDLE_VALUE - && !(inh_handles->flags[newfd] & KEEP_OPEN_IN_PARENT) - && !CloseHandle (inh_handles->handles[newfd])) + if (inh_handles->ih[newfd].handle != INVALID_HANDLE_VALUE + && !(inh_handles->ih[newfd].flags & KEEP_OPEN_IN_PARENT) + && !CloseHandle (inh_handles->ih[newfd].handle)) { errno = EIO; return -1; } /* Duplicate the handle, so that a forthcoming do_close action on oldfd has no effect on newfd. */ - if (!DuplicateHandle (curr_process, inh_handles->handles[oldfd], - curr_process, &inh_handles->handles[newfd], + if (!DuplicateHandle (curr_process, inh_handles->ih[oldfd].handle, + curr_process, &inh_handles->ih[newfd].handle, 0, TRUE, DUPLICATE_SAME_ACCESS)) { errno = EBADF; /* arbitrary */ return -1; } - inh_handles->flags[newfd] = - (unsigned char) inh_handles->flags[oldfd] | KEEP_OPEN_IN_CHILD; + inh_handles->ih[newfd].flags = + (unsigned char) inh_handles->ih[oldfd].flags | KEEP_OPEN_IN_CHILD; } return 0; } @@ -485,18 +473,18 @@ static int do_close (struct inheritable_handles *inh_handles, int fd) { if (!(fd >= 0 && fd < inh_handles->count - && inh_handles->handles[fd] != INVALID_HANDLE_VALUE)) + && inh_handles->ih[fd].handle != INVALID_HANDLE_VALUE)) { errno = EBADF; return -1; } - if (!(inh_handles->flags[fd] & KEEP_OPEN_IN_PARENT) - && !CloseHandle (inh_handles->handles[fd])) + if (!(inh_handles->ih[fd].flags & KEEP_OPEN_IN_PARENT) + && !CloseHandle (inh_handles->ih[fd].handle)) { errno = EIO; return -1; } - inh_handles->handles[fd] = INVALID_HANDLE_VALUE; + inh_handles->ih[fd].handle = INVALID_HANDLE_VALUE; return 0; } diff --git a/lib/windows-spawn.c b/lib/windows-spawn.c index d38d1dc911..e81fc168cf 100644 --- a/lib/windows-spawn.c +++ b/lib/windows-spawn.c @@ -346,31 +346,23 @@ init_inheritable_handles (struct inheritable_handles *inh_handles, } /* Note: handles_count >= 3. */ - /* Allocate the arrays. */ + /* Allocate the array. */ size_t handles_allocated = handles_count; - HANDLE *handles_array = - (HANDLE *) malloc (handles_allocated * sizeof (HANDLE)); - if (handles_array == NULL) + struct IHANDLE *ih = + (struct IHANDLE *) malloc (handles_allocated * sizeof (struct IHANDLE)); + if (ih == NULL) { errno = ENOMEM; return -1; } - unsigned short *flags_array = - (unsigned short *) malloc (handles_allocated * sizeof (unsigned short)); - if (flags_array == NULL) - { - free (handles_array); - errno = ENOMEM; - return -1; - } - /* Fill in the two arrays. */ + /* Fill in the array. */ { HANDLE curr_process = (duplicate ? GetCurrentProcess () : INVALID_HANDLE_VALUE); unsigned int fd; for (fd = 0; fd < handles_count; fd++) { - handles_array[fd] = INVALID_HANDLE_VALUE; + ih[fd].handle = INVALID_HANDLE_VALUE; /* _get_osfhandle */ HANDLE handle = (HANDLE) _get_osfhandle (fd); @@ -388,26 +380,25 @@ init_inheritable_handles (struct inheritable_handles *inh_handles, if ((hflags & HANDLE_FLAG_INHERIT) != 0) { /* Instead of duplicating it, just mark it as shared. */ - handles_array[fd] = handle; - flags_array[fd] = KEEP_OPEN_IN_PARENT | KEEP_OPEN_IN_CHILD; + ih[fd].handle = handle; + ih[fd].flags = KEEP_OPEN_IN_PARENT | KEEP_OPEN_IN_CHILD; } else { if (!DuplicateHandle (curr_process, handle, - curr_process, &handles_array[fd], + curr_process, &ih[fd].handle, 0, TRUE, DUPLICATE_SAME_ACCESS)) { unsigned int i; for (i = 0; i < fd; i++) - if (handles_array[i] != INVALID_HANDLE_VALUE - && !(flags_array[i] & KEEP_OPEN_IN_PARENT)) - CloseHandle (handles_array[i]); - free (flags_array); - free (handles_array); + if (ih[i].handle != INVALID_HANDLE_VALUE + && !(ih[i].flags & KEEP_OPEN_IN_PARENT)) + CloseHandle (ih[i].handle); + free (ih); errno = EBADF; /* arbitrary */ return -1; } - flags_array[fd] = 0; + ih[fd].flags = 0; } } else @@ -415,8 +406,8 @@ init_inheritable_handles (struct inheritable_handles *inh_handles, if ((hflags & HANDLE_FLAG_INHERIT) != 0) { /* fd denotes an inheritable descriptor. */ - handles_array[fd] = handle; - flags_array[fd] = KEEP_OPEN_IN_CHILD; + ih[fd].handle = handle; + ih[fd].flags = KEEP_OPEN_IN_CHILD; } } } @@ -427,8 +418,7 @@ init_inheritable_handles (struct inheritable_handles *inh_handles, /* Return the result. */ inh_handles->count = handles_count; inh_handles->allocated = handles_allocated; - inh_handles->handles = handles_array; - inh_handles->flags = flags_array; + inh_handles->ih = ih; return 0; } @@ -439,9 +429,9 @@ compose_handles_block (const struct inheritable_handles *inh_handles, /* STARTUPINFO */ sinfo->dwFlags = STARTF_USESTDHANDLES; - sinfo->hStdInput = inh_handles->handles[0]; - sinfo->hStdOutput = inh_handles->handles[1]; - sinfo->hStdError = inh_handles->handles[2]; + sinfo->hStdInput = inh_handles->ih[0].handle; + sinfo->hStdOutput = inh_handles->ih[1].handle; + sinfo->hStdError = inh_handles->ih[2].handle; /* On newer versions of Windows, more file descriptors / handles than the first three can be passed. @@ -492,11 +482,11 @@ compose_handles_block (const struct inheritable_handles *inh_handles, handles_aligned[fd] = INVALID_HANDLE_VALUE; flags[fd] = 0; - HANDLE handle = inh_handles->handles[fd]; + HANDLE handle = inh_handles->ih[fd].handle; if (handle != INVALID_HANDLE_VALUE /* The first three are possibly already passed above. But they need to passed here as well, if they have some flags. */ - && (fd >= 3 || (unsigned char) inh_handles->flags[fd] != 0)) + && (fd >= 3 || (unsigned char) inh_handles->ih[fd].flags != 0)) { DWORD hflags; /* GetHandleInformation @@ -511,7 +501,7 @@ compose_handles_block (const struct inheritable_handles *inh_handles, flags[fd] = 1. But on ReactOS or Wine, adding the bit that indicates the handle type may be necessary. So, just do it everywhere. */ - flags[fd] = 1 | (unsigned char) inh_handles->flags[fd]; + flags[fd] = 1 | (unsigned char) inh_handles->ih[fd].flags; switch (GetFileType (handle)) { case FILE_TYPE_CHAR: @@ -543,8 +533,7 @@ compose_handles_block (const struct inheritable_handles *inh_handles, void free_inheritable_handles (struct inheritable_handles *inh_handles) { - free (inh_handles->flags); - free (inh_handles->handles); + free (inh_handles->ih); } int @@ -640,12 +629,12 @@ spawnpvech (int mode, errno = saved_errno; return -1; } - inh_handles.handles[0] = stdin_handle; - inh_handles.flags[0] = KEEP_OPEN_IN_CHILD; - inh_handles.handles[1] = stdout_handle; - inh_handles.flags[1] = KEEP_OPEN_IN_CHILD; - inh_handles.handles[2] = stderr_handle; - inh_handles.flags[2] = KEEP_OPEN_IN_CHILD; + inh_handles.ih[0].handle = stdin_handle; + inh_handles.ih[0].flags = KEEP_OPEN_IN_CHILD; + inh_handles.ih[1].handle = stdout_handle; + inh_handles.ih[1].flags = KEEP_OPEN_IN_CHILD; + inh_handles.ih[2].handle = stderr_handle; + inh_handles.ih[2].flags = KEEP_OPEN_IN_CHILD; /* CreateProcess */ diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h index fdb45a4f15..5cc770b59d 100644 --- a/lib/windows-spawn.h +++ b/lib/windows-spawn.h @@ -85,6 +85,24 @@ extern char * compose_envblock (const char * const *envp) _GL_ATTRIBUTE_MALLOC _GL_ATTRIBUTE_DEALLOC_FREE; +/* An inheritable handle with some additional information. */ +struct IHANDLE +{ + /* Either INVALID_HANDLE_VALUE or an inheritable handle. */ + HANDLE handle; + /* Only relevant if handle != INVALID_HANDLE_VALUE. + It is a bit mask consisting of: + - 32 for O_APPEND. + - KEEP_OPEN_IN_CHILD if the handle is scheduled to be preserved in the + child process. + - KEEP_OPEN_IN_PARENT if the handle is shared with (and needs to be kept + open in) the parent process. + */ + unsigned short flags; + #define KEEP_OPEN_IN_CHILD 0x100 + #define KEEP_OPEN_IN_PARENT 0x200 +}; + /* This struct keeps track of which handles to potentially pass to a subprocess, and with which flags. All of the handles here are inheritable. Regarding handle inheritance, see @@ -98,21 +116,8 @@ struct inheritable_handles size_t count; /* The number of allocated entries in the two arrays below. */ size_t allocated; - /* handles[0..count-1] are the occupied entries. - handles[fd] is either INVALID_HANDLE_VALUE or an inheritable handle. */ - HANDLE *handles; - /* flags[0..count-1] are the occupied entries. - flags[fd] is only relevant if handles[fd] != INVALID_HANDLE_VALUE. - It is a bit mask consisting of: - - 32 for O_APPEND. - - KEEP_OPEN_IN_CHILD if handles[fd] is scheduled to be preserved in the - child process. - - KEEP_OPEN_IN_PARENT if handles[fd] is shared with (and needs to be kept - open in) the parent process. - */ - unsigned short *flags; - #define KEEP_OPEN_IN_CHILD 0x100 - #define KEEP_OPEN_IN_PARENT 0x200 + /* ih[0..count-1] are the occupied entries. */ + struct IHANDLE *ih; }; /* Initializes a set of inheritable handles, filling in all or part of the -- 2.34.1