From: Marc-André Lureau <marcandre.lureau@redhat.com>
Simplify qio_channel_command_new_spawn() with GSpawn API. This will
allow to build for WIN32 in the following patches.
As pointed out by Daniel Berrangé: there is a change in semantics here
too. The current code only touches stdin/stdout/stderr. Any other FDs
which do NOT have O_CLOEXEC set will be inherited. With the new code,
all FDs except stdin/out/err will be explicitly closed, because we don't
set the flag G_SPAWN_LEAVE_DESCRIPTORS_OPEN. The only place we use
QIOChannelCommand today is the migration exec: protocol, and that is
only declared to use stdin/stdout.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20221006113657.2656108-5-marcandre.lureau@redhat.com>
---
include/io/channel-command.h | 2 +-
io/channel-command.c | 105 ++++++-----------------------------
2 files changed, 19 insertions(+), 88 deletions(-)
diff --git a/include/io/channel-command.h b/include/io/channel-command.h
index 305ac1d280..8dc58273c0 100644
--- a/include/io/channel-command.h
+++ b/include/io/channel-command.h
@@ -41,7 +41,7 @@ struct QIOChannelCommand {
QIOChannel parent;
int writefd;
int readfd;
- pid_t pid;
+ GPid pid;
};
diff --git a/io/channel-command.c b/io/channel-command.c
index 9f2f4a1793..f84d1f03a0 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -31,7 +31,7 @@
* qio_channel_command_new_pid:
* @writefd: the FD connected to the command's stdin
* @readfd: the FD connected to the command's stdout
- * @pid: the PID of the running child command
+ * @pid: the PID/HANDLE of the running child command
* @errp: pointer to a NULL-initialized error object
*
* Create a channel for performing I/O with the
@@ -50,7 +50,7 @@
static QIOChannelCommand *
qio_channel_command_new_pid(int writefd,
int readfd,
- pid_t pid)
+ GPid pid)
{
QIOChannelCommand *ioc;
@@ -69,94 +69,24 @@ qio_channel_command_new_spawn(const char *const argv[],
int flags,
Error **errp)
{
- pid_t pid = -1;
- int stdinfd[2] = { -1, -1 };
- int stdoutfd[2] = { -1, -1 };
- int devnull = -1;
- bool stdinnull = false, stdoutnull = false;
- QIOChannelCommand *ioc;
+ g_autoptr(GError) err = NULL;
+ GPid pid = 0;
+ GSpawnFlags gflags = G_SPAWN_CLOEXEC_PIPES | G_SPAWN_DO_NOT_REAP_CHILD;
+ int stdinfd = -1, stdoutfd = -1;
flags = flags & O_ACCMODE;
-
- if (flags == O_RDONLY) {
- stdinnull = true;
- }
- if (flags == O_WRONLY) {
- stdoutnull = true;
- }
-
- if (stdinnull || stdoutnull) {
- devnull = open("/dev/null", O_RDWR);
- if (devnull < 0) {
- error_setg_errno(errp, errno,
- "Unable to open /dev/null");
- goto error;
- }
- }
-
- if ((!stdinnull && !g_unix_open_pipe(stdinfd, FD_CLOEXEC, NULL)) ||
- (!stdoutnull && !g_unix_open_pipe(stdoutfd, FD_CLOEXEC, NULL))) {
- error_setg_errno(errp, errno,
- "Unable to open pipe");
- goto error;
- }
-
- pid = qemu_fork(errp);
- if (pid < 0) {
- goto error;
- }
-
- if (pid == 0) { /* child */
- dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);
- dup2(stdoutnull ? devnull : stdoutfd[1], STDOUT_FILENO);
- /* Leave stderr connected to qemu's stderr */
-
- if (!stdinnull) {
- close(stdinfd[0]);
- close(stdinfd[1]);
- }
- if (!stdoutnull) {
- close(stdoutfd[0]);
- close(stdoutfd[1]);
- }
- if (devnull != -1) {
- close(devnull);
- }
-
- execv(argv[0], (char * const *)argv);
- _exit(1);
+ gflags |= flags == O_WRONLY ? G_SPAWN_STDOUT_TO_DEV_NULL : 0;
+
+ if (!g_spawn_async_with_pipes(NULL, (char **)argv, NULL, gflags, NULL, NULL,
+ &pid,
+ flags == O_RDONLY ? NULL : &stdinfd,
+ flags == O_WRONLY ? NULL : &stdoutfd,
+ NULL, &err)) {
+ error_setg(errp, "%s", err->message);
+ return NULL;
}
- if (!stdinnull) {
- close(stdinfd[0]);
- }
- if (!stdoutnull) {
- close(stdoutfd[1]);
- }
-
- ioc = qio_channel_command_new_pid(stdinnull ? devnull : stdinfd[1],
- stdoutnull ? devnull : stdoutfd[0],
- pid);
- trace_qio_channel_command_new_spawn(ioc, argv[0], flags);
- return ioc;
-
- error:
- if (devnull != -1) {
- close(devnull);
- }
- if (stdinfd[0] != -1) {
- close(stdinfd[0]);
- }
- if (stdinfd[1] != -1) {
- close(stdinfd[1]);
- }
- if (stdoutfd[0] != -1) {
- close(stdoutfd[0]);
- }
- if (stdoutfd[1] != -1) {
- close(stdoutfd[1]);
- }
- return NULL;
+ return qio_channel_command_new_pid(stdinfd, stdoutfd, pid);
}
#else /* WIN32 */
@@ -221,7 +151,7 @@ static void qio_channel_command_init(Object *obj)
QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
ioc->readfd = -1;
ioc->writefd = -1;
- ioc->pid = -1;
+ ioc->pid = 0;
}
static void qio_channel_command_finalize(Object *obj)
@@ -239,6 +169,7 @@ static void qio_channel_command_finalize(Object *obj)
#ifndef WIN32
qio_channel_command_abort(ioc, NULL);
#endif
+ g_spawn_close_pid(ioc->pid);
}
}
--
2.37.3
Am 12.10.22 um 18:04 schrieb marcandre.lureau@redhat.com:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Simplify qio_channel_command_new_spawn() with GSpawn API. This will
> allow to build for WIN32 in the following patches.
>
> As pointed out by Daniel Berrangé: there is a change in semantics here
> too. The current code only touches stdin/stdout/stderr. Any other FDs
> which do NOT have O_CLOEXEC set will be inherited. With the new code,
> all FDs except stdin/out/err will be explicitly closed, because we don't
> set the flag G_SPAWN_LEAVE_DESCRIPTORS_OPEN. The only place we use
> QIOChannelCommand today is the migration exec: protocol, and that is
> only declared to use stdin/stdout.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20221006113657.2656108-5-marcandre.lureau@redhat.com>
> ---
> include/io/channel-command.h | 2 +-
> io/channel-command.c | 105 ++++++-----------------------------
> 2 files changed, 19 insertions(+), 88 deletions(-)
>
> diff --git a/include/io/channel-command.h b/include/io/channel-command.h
> index 305ac1d280..8dc58273c0 100644
> --- a/include/io/channel-command.h
> +++ b/include/io/channel-command.h
> @@ -41,7 +41,7 @@ struct QIOChannelCommand {
> QIOChannel parent;
> int writefd;
> int readfd;
> - pid_t pid;
> + GPid pid;
GPid is a pointer for Windows now. Therefore code like pid > 0 is no
longer valid. As the new initial value is no longer -1, but 0 now, this
patch might fix the code for Windows:
diff --git a/io/channel-command.c b/io/channel-command.c
index 74516252ba..8850a374f1 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -175,7 +175,7 @@ static void qio_channel_command_finalize(Object *obj)
close(ioc->writefd);
}
ioc->writefd = ioc->readfd = -1;
- if (ioc->pid > 0) {
+ if (ioc->pid != 0) {
qio_channel_command_abort(ioc, NULL);
g_spawn_close_pid(ioc->pid);
}
> };
>
>
> diff --git a/io/channel-command.c b/io/channel-command.c
> index 9f2f4a1793..f84d1f03a0 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -31,7 +31,7 @@
> * qio_channel_command_new_pid:
> * @writefd: the FD connected to the command's stdin
> * @readfd: the FD connected to the command's stdout
> - * @pid: the PID of the running child command
> + * @pid: the PID/HANDLE of the running child command
> * @errp: pointer to a NULL-initialized error object
> *
> * Create a channel for performing I/O with the
> @@ -50,7 +50,7 @@
> static QIOChannelCommand *
> qio_channel_command_new_pid(int writefd,
> int readfd,
> - pid_t pid)
> + GPid pid)
> {
> QIOChannelCommand *ioc;
>
> @@ -69,94 +69,24 @@ qio_channel_command_new_spawn(const char *const argv[],
> int flags,
> Error **errp)
> {
> - pid_t pid = -1;
> - int stdinfd[2] = { -1, -1 };
> - int stdoutfd[2] = { -1, -1 };
> - int devnull = -1;
> - bool stdinnull = false, stdoutnull = false;
> - QIOChannelCommand *ioc;
> + g_autoptr(GError) err = NULL;
> + GPid pid = 0;
> + GSpawnFlags gflags = G_SPAWN_CLOEXEC_PIPES | G_SPAWN_DO_NOT_REAP_CHILD;
> + int stdinfd = -1, stdoutfd = -1;
>
> flags = flags & O_ACCMODE;
> -
> - if (flags == O_RDONLY) {
> - stdinnull = true;
> - }
> - if (flags == O_WRONLY) {
> - stdoutnull = true;
> - }
> -
> - if (stdinnull || stdoutnull) {
> - devnull = open("/dev/null", O_RDWR);
> - if (devnull < 0) {
> - error_setg_errno(errp, errno,
> - "Unable to open /dev/null");
> - goto error;
> - }
> - }
> -
> - if ((!stdinnull && !g_unix_open_pipe(stdinfd, FD_CLOEXEC, NULL)) ||
> - (!stdoutnull && !g_unix_open_pipe(stdoutfd, FD_CLOEXEC, NULL))) {
> - error_setg_errno(errp, errno,
> - "Unable to open pipe");
> - goto error;
> - }
> -
> - pid = qemu_fork(errp);
> - if (pid < 0) {
> - goto error;
> - }
> -
> - if (pid == 0) { /* child */
> - dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);
> - dup2(stdoutnull ? devnull : stdoutfd[1], STDOUT_FILENO);
> - /* Leave stderr connected to qemu's stderr */
> -
> - if (!stdinnull) {
> - close(stdinfd[0]);
> - close(stdinfd[1]);
> - }
> - if (!stdoutnull) {
> - close(stdoutfd[0]);
> - close(stdoutfd[1]);
> - }
> - if (devnull != -1) {
> - close(devnull);
> - }
> -
> - execv(argv[0], (char * const *)argv);
> - _exit(1);
> + gflags |= flags == O_WRONLY ? G_SPAWN_STDOUT_TO_DEV_NULL : 0;
> +
> + if (!g_spawn_async_with_pipes(NULL, (char **)argv, NULL, gflags, NULL, NULL,
> + &pid,
> + flags == O_RDONLY ? NULL : &stdinfd,
> + flags == O_WRONLY ? NULL : &stdoutfd,
> + NULL, &err)) {
> + error_setg(errp, "%s", err->message);
> + return NULL;
> }
>
> - if (!stdinnull) {
> - close(stdinfd[0]);
> - }
> - if (!stdoutnull) {
> - close(stdoutfd[1]);
> - }
> -
> - ioc = qio_channel_command_new_pid(stdinnull ? devnull : stdinfd[1],
> - stdoutnull ? devnull : stdoutfd[0],
> - pid);
> - trace_qio_channel_command_new_spawn(ioc, argv[0], flags);
> - return ioc;
> -
> - error:
> - if (devnull != -1) {
> - close(devnull);
> - }
> - if (stdinfd[0] != -1) {
> - close(stdinfd[0]);
> - }
> - if (stdinfd[1] != -1) {
> - close(stdinfd[1]);
> - }
> - if (stdoutfd[0] != -1) {
> - close(stdoutfd[0]);
> - }
> - if (stdoutfd[1] != -1) {
> - close(stdoutfd[1]);
> - }
> - return NULL;
> + return qio_channel_command_new_pid(stdinfd, stdoutfd, pid);
> }
>
> #else /* WIN32 */
> @@ -221,7 +151,7 @@ static void qio_channel_command_init(Object *obj)
> QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
> ioc->readfd = -1;
> ioc->writefd = -1;
> - ioc->pid = -1;
> + ioc->pid = 0;
> }
>
> static void qio_channel_command_finalize(Object *obj)
> @@ -239,6 +169,7 @@ static void qio_channel_command_finalize(Object *obj)
> #ifndef WIN32
> qio_channel_command_abort(ioc, NULL);
> #endif
> + g_spawn_close_pid(ioc->pid);
> }
> }
>
Hi Stefan
On Sat, Oct 29, 2022 at 11:12 PM Stefan Weil <sw@weilnetz.de> wrote:
> Am 12.10.22 um 18:04 schrieb marcandre.lureau@redhat.com:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Simplify qio_channel_command_new_spawn() with GSpawn API. This will
> > allow to build for WIN32 in the following patches.
> >
> > As pointed out by Daniel Berrangé: there is a change in semantics here
> > too. The current code only touches stdin/stdout/stderr. Any other FDs
> > which do NOT have O_CLOEXEC set will be inherited. With the new code,
> > all FDs except stdin/out/err will be explicitly closed, because we don't
> > set the flag G_SPAWN_LEAVE_DESCRIPTORS_OPEN. The only place we use
> > QIOChannelCommand today is the migration exec: protocol, and that is
> > only declared to use stdin/stdout.
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Message-Id: <20221006113657.2656108-5-marcandre.lureau@redhat.com>
> > ---
> > include/io/channel-command.h | 2 +-
> > io/channel-command.c | 105 ++++++-----------------------------
> > 2 files changed, 19 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/io/channel-command.h b/include/io/channel-command.h
> > index 305ac1d280..8dc58273c0 100644
> > --- a/include/io/channel-command.h
> > +++ b/include/io/channel-command.h
> > @@ -41,7 +41,7 @@ struct QIOChannelCommand {
> > QIOChannel parent;
> > int writefd;
> > int readfd;
> > - pid_t pid;
> > + GPid pid;
>
>
> GPid is a pointer for Windows now. Therefore code like pid > 0 is no
> longer valid. As the new initial value is no longer -1, but 0 now, this
> patch might fix the code for Windows:
>
> diff --git a/io/channel-command.c b/io/channel-command.c
> index 74516252ba..8850a374f1 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -175,7 +175,7 @@ static void qio_channel_command_finalize(Object *obj)
> close(ioc->writefd);
> }
> ioc->writefd = ioc->readfd = -1;
> - if (ioc->pid > 0) {
> + if (ioc->pid != 0) {
> qio_channel_command_abort(ioc, NULL);
> g_spawn_close_pid(ioc->pid);
> }
>
Hmm, GPid is an "int" on unix, "void*" on windows. I think the current code
should work fine. Do you see really an issue? Your change looks correct to
me too, can you send a proper patch with details?
thanks
>
>
> > };
> >
> >
> > diff --git a/io/channel-command.c b/io/channel-command.c
> > index 9f2f4a1793..f84d1f03a0 100644
> > --- a/io/channel-command.c
> > +++ b/io/channel-command.c
> > @@ -31,7 +31,7 @@
> > * qio_channel_command_new_pid:
> > * @writefd: the FD connected to the command's stdin
> > * @readfd: the FD connected to the command's stdout
> > - * @pid: the PID of the running child command
> > + * @pid: the PID/HANDLE of the running child command
> > * @errp: pointer to a NULL-initialized error object
> > *
> > * Create a channel for performing I/O with the
> > @@ -50,7 +50,7 @@
> > static QIOChannelCommand *
> > qio_channel_command_new_pid(int writefd,
> > int readfd,
> > - pid_t pid)
> > + GPid pid)
> > {
> > QIOChannelCommand *ioc;
> >
> > @@ -69,94 +69,24 @@ qio_channel_command_new_spawn(const char *const
> argv[],
> > int flags,
> > Error **errp)
> > {
> > - pid_t pid = -1;
> > - int stdinfd[2] = { -1, -1 };
> > - int stdoutfd[2] = { -1, -1 };
> > - int devnull = -1;
> > - bool stdinnull = false, stdoutnull = false;
> > - QIOChannelCommand *ioc;
> > + g_autoptr(GError) err = NULL;
> > + GPid pid = 0;
> > + GSpawnFlags gflags = G_SPAWN_CLOEXEC_PIPES |
> G_SPAWN_DO_NOT_REAP_CHILD;
> > + int stdinfd = -1, stdoutfd = -1;
> >
> > flags = flags & O_ACCMODE;
> > -
> > - if (flags == O_RDONLY) {
> > - stdinnull = true;
> > - }
> > - if (flags == O_WRONLY) {
> > - stdoutnull = true;
> > - }
> > -
> > - if (stdinnull || stdoutnull) {
> > - devnull = open("/dev/null", O_RDWR);
> > - if (devnull < 0) {
> > - error_setg_errno(errp, errno,
> > - "Unable to open /dev/null");
> > - goto error;
> > - }
> > - }
> > -
> > - if ((!stdinnull && !g_unix_open_pipe(stdinfd, FD_CLOEXEC, NULL)) ||
> > - (!stdoutnull && !g_unix_open_pipe(stdoutfd, FD_CLOEXEC, NULL)))
> {
> > - error_setg_errno(errp, errno,
> > - "Unable to open pipe");
> > - goto error;
> > - }
> > -
> > - pid = qemu_fork(errp);
> > - if (pid < 0) {
> > - goto error;
> > - }
> > -
> > - if (pid == 0) { /* child */
> > - dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);
> > - dup2(stdoutnull ? devnull : stdoutfd[1], STDOUT_FILENO);
> > - /* Leave stderr connected to qemu's stderr */
> > -
> > - if (!stdinnull) {
> > - close(stdinfd[0]);
> > - close(stdinfd[1]);
> > - }
> > - if (!stdoutnull) {
> > - close(stdoutfd[0]);
> > - close(stdoutfd[1]);
> > - }
> > - if (devnull != -1) {
> > - close(devnull);
> > - }
> > -
> > - execv(argv[0], (char * const *)argv);
> > - _exit(1);
> > + gflags |= flags == O_WRONLY ? G_SPAWN_STDOUT_TO_DEV_NULL : 0;
> > +
> > + if (!g_spawn_async_with_pipes(NULL, (char **)argv, NULL, gflags,
> NULL, NULL,
> > + &pid,
> > + flags == O_RDONLY ? NULL : &stdinfd,
> > + flags == O_WRONLY ? NULL : &stdoutfd,
> > + NULL, &err)) {
> > + error_setg(errp, "%s", err->message);
> > + return NULL;
> > }
> >
> > - if (!stdinnull) {
> > - close(stdinfd[0]);
> > - }
> > - if (!stdoutnull) {
> > - close(stdoutfd[1]);
> > - }
> > -
> > - ioc = qio_channel_command_new_pid(stdinnull ? devnull : stdinfd[1],
> > - stdoutnull ? devnull :
> stdoutfd[0],
> > - pid);
> > - trace_qio_channel_command_new_spawn(ioc, argv[0], flags);
> > - return ioc;
> > -
> > - error:
> > - if (devnull != -1) {
> > - close(devnull);
> > - }
> > - if (stdinfd[0] != -1) {
> > - close(stdinfd[0]);
> > - }
> > - if (stdinfd[1] != -1) {
> > - close(stdinfd[1]);
> > - }
> > - if (stdoutfd[0] != -1) {
> > - close(stdoutfd[0]);
> > - }
> > - if (stdoutfd[1] != -1) {
> > - close(stdoutfd[1]);
> > - }
> > - return NULL;
> > + return qio_channel_command_new_pid(stdinfd, stdoutfd, pid);
> > }
> >
> > #else /* WIN32 */
> > @@ -221,7 +151,7 @@ static void qio_channel_command_init(Object *obj)
> > QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
> > ioc->readfd = -1;
> > ioc->writefd = -1;
> > - ioc->pid = -1;
> > + ioc->pid = 0;
> > }
> >
> > static void qio_channel_command_finalize(Object *obj)
> > @@ -239,6 +169,7 @@ static void qio_channel_command_finalize(Object *obj)
> > #ifndef WIN32
> > qio_channel_command_abort(ioc, NULL);
> > #endif
> > + g_spawn_close_pid(ioc->pid);
> > }
> > }
> >
>
© 2016 - 2026 Red Hat, Inc.