| Message ID | 20181114123643.24091-4-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: slirp: make it again a standalone project | expand |
On Wed, Nov 14, 2018 at 04:36:05PM +0400, Marc-André Lureau wrote: > Use g_spawn_async_with_fds() to setup the child. > > GSpawn handles reaping the child, and closing parent file descriptors. The g_spawn* family of APIs is portable to Win32, which the current fork/exec code in slirp is not. So I'm curious if this conversion is sufficient to make this part of slirp work on Win32, or if further portability problems remain. If it does work on Win32, then you can also delete the stub fork_exec() impl the code currently has. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > slirp/misc.c | 75 +++++++++++++++++++++++++--------------------------- > 1 file changed, 36 insertions(+), 39 deletions(-) > > diff --git a/slirp/misc.c b/slirp/misc.c > index 7362e14339..7972b9b05b 100644 > --- a/slirp/misc.c > +++ b/slirp/misc.c > @@ -129,56 +129,53 @@ err: > return -1; > } > > +static void > +fork_exec_child_setup(gpointer data) > +{ > + setsid(); > +} > + > int > fork_exec(struct socket *so, const char *ex) > { > - char **argv; > - int opt, c, sp[2]; > - pid_t pid; > + GError *err = NULL; > + char **argv; > + int opt, sp[2]; > > - DEBUG_CALL("fork_exec"); > - DEBUG_ARG("so = %p", so); > - DEBUG_ARG("ex = %p", ex); > + DEBUG_CALL("fork_exec"); > + DEBUG_ARG("so = %p", so); > + DEBUG_ARG("ex = %p", ex); > > if (slirp_socketpair_with_oob(sp) < 0) { > return 0; > } > > - pid = fork(); > - switch(pid) { > - case -1: > - error_report("Error: fork failed: %s", strerror(errno)); > - closesocket(sp[0]); > - closesocket(sp[1]); > - return 0; > - > - case 0: > - setsid(); > - dup2(sp[1], 0); > - dup2(sp[1], 1); > - dup2(sp[1], 2); > - for (c = getdtablesize() - 1; c >= 3; c--) > - close(c); > + argv = g_strsplit(ex, " ", -1); > + g_spawn_async_with_fds(NULL /* cwd */, > + argv, > + NULL /* env */, > + G_SPAWN_SEARCH_PATH, > + fork_exec_child_setup, NULL /* data */, > + NULL /* child_pid */, > + sp[1], sp[1], sp[1], > + &err); > + g_strfreev(argv); > > - argv = g_strsplit(ex, " ", -1); > - execvp(argv[0], (char **)argv); > - > - /* Ooops, failed, let's tell the user why */ > - fprintf(stderr, "Error: execvp of %s failed: %s\n", > - argv[0], strerror(errno)); > - close(0); close(1); close(2); /* XXX */ > - exit(1); > + if (err) { > + error_report("%s", err->message); > + g_error_free(err); > + closesocket(sp[0]); > + closesocket(sp[1]); > + return 0; > + } > > - default: > - so->s = sp[0]; > - closesocket(sp[1]); > - qemu_add_child_watch(pid); > - socket_set_fast_reuse(so->s); > - opt = 1; > - qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int)); > - qemu_set_nonblock(so->s); > - return 1; > - } > + so->s = sp[0]; > + closesocket(sp[1]); > + socket_set_fast_reuse(so->s); > + opt = 1; > + qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int)); > + qemu_set_nonblock(so->s); > + return 1; > } > #endif > > -- > 2.19.1.708.g4ede3d42df > > Regards, Daniel
Marc-André Lureau, le mer. 14 nov. 2018 16:36:05 +0400, a ecrit: > Use g_spawn_async_with_fds() to setup the child. > > GSpawn handles reaping the child, and closing parent file descriptors. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Applied to my slirp-2 tree, thanks! Samuel
Hello, Daniel P. Berrangé, le mer. 14 nov. 2018 14:22:34 +0000, a ecrit: > On Wed, Nov 14, 2018 at 04:36:05PM +0400, Marc-André Lureau wrote: > > Use g_spawn_async_with_fds() to setup the child. > > > > GSpawn handles reaping the child, and closing parent file descriptors. > > The g_spawn* family of APIs is portable to Win32, which the current > fork/exec code in slirp is not. > > So I'm curious if this conversion is sufficient to make this part > of slirp work on Win32, or if further portability problems remain. > If it does work on Win32, then you can also delete the stub > fork_exec() impl the code currently has. Mmm, I don't think any portability issue remains. SO_OOBINLINE is used in other places, the rest is portable. There is the setsid() call which may just not make sense on Windows, but we could just disable it there. I have pushed to https://people.debian.org/~sthibault/qemu.git slirp-2-win what it could look like. I don't have a windows box off hand, could somebody try it to confirm that it builds? Samuel
On 11/19/18 4:59 PM, Samuel Thibault wrote: > Mmm, I don't think any portability issue remains. SO_OOBINLINE is used > in other places, the rest is portable. There is the setsid() call which > may just not make sense on Windows, but we could just disable it there. > > I have pushed to > > https://people.debian.org/~sthibault/qemu.git slirp-2-win > > what it could look like. I don't have a windows box off hand, could > somebody try it to confirm that it builds? You could try 'make docker-test-mingw@fedora' to at least try building for a Windows environment (but that doesn't say much about testing the resulting binary).
diff --git a/slirp/misc.c b/slirp/misc.c index 7362e14339..7972b9b05b 100644 --- a/slirp/misc.c +++ b/slirp/misc.c @@ -129,56 +129,53 @@ err: return -1; } +static void +fork_exec_child_setup(gpointer data) +{ + setsid(); +} + int fork_exec(struct socket *so, const char *ex) { - char **argv; - int opt, c, sp[2]; - pid_t pid; + GError *err = NULL; + char **argv; + int opt, sp[2]; - DEBUG_CALL("fork_exec"); - DEBUG_ARG("so = %p", so); - DEBUG_ARG("ex = %p", ex); + DEBUG_CALL("fork_exec"); + DEBUG_ARG("so = %p", so); + DEBUG_ARG("ex = %p", ex); if (slirp_socketpair_with_oob(sp) < 0) { return 0; } - pid = fork(); - switch(pid) { - case -1: - error_report("Error: fork failed: %s", strerror(errno)); - closesocket(sp[0]); - closesocket(sp[1]); - return 0; - - case 0: - setsid(); - dup2(sp[1], 0); - dup2(sp[1], 1); - dup2(sp[1], 2); - for (c = getdtablesize() - 1; c >= 3; c--) - close(c); + argv = g_strsplit(ex, " ", -1); + g_spawn_async_with_fds(NULL /* cwd */, + argv, + NULL /* env */, + G_SPAWN_SEARCH_PATH, + fork_exec_child_setup, NULL /* data */, + NULL /* child_pid */, + sp[1], sp[1], sp[1], + &err); + g_strfreev(argv); - argv = g_strsplit(ex, " ", -1); - execvp(argv[0], (char **)argv); - - /* Ooops, failed, let's tell the user why */ - fprintf(stderr, "Error: execvp of %s failed: %s\n", - argv[0], strerror(errno)); - close(0); close(1); close(2); /* XXX */ - exit(1); + if (err) { + error_report("%s", err->message); + g_error_free(err); + closesocket(sp[0]); + closesocket(sp[1]); + return 0; + } - default: - so->s = sp[0]; - closesocket(sp[1]); - qemu_add_child_watch(pid); - socket_set_fast_reuse(so->s); - opt = 1; - qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int)); - qemu_set_nonblock(so->s); - return 1; - } + so->s = sp[0]; + closesocket(sp[1]); + socket_set_fast_reuse(so->s); + opt = 1; + qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int)); + qemu_set_nonblock(so->s); + return 1; } #endif
Use g_spawn_async_with_fds() to setup the child. GSpawn handles reaping the child, and closing parent file descriptors. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- slirp/misc.c | 75 +++++++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 39 deletions(-)