On Fri, Oct 18, 2019 at 05:55:10PM +0800, Wang Yechao wrote:
> Libvirtd has set SIGPIPE to ignored, and virFork resets all signal
> handlers to the defaults. But child process may write logs to
> stderr/stdout, that may generate SIGPIPE if journald has stopped.
>
> So set SIGPIPE to a dummy no-op handler before unmask signals in
> virFork. And there is no need to set SIGPIPE ignored before running
> hooks in virExec. At last, set SIGPIPE to defaults before execve.
>
> Signed-off-by: Wang Yechao <wang.yechao...@zte.com.cn>
> ---
> v3 patch:
> https://www.redhat.com/archives/libvir-list/2019-October/msg00934.html
>
> Changes in v4:
> - don't block SIGPIPE, ignore it when invoke VIR_FORCE_CLOSE and
> virCommandMassClose
>
> Changes in v5:
> - chang from SIG_IGN to a no-op handler in child process
> ---
> src/util/vircommand.c | 27 ++++++++++-----------------
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 93b3dd2..c13739c 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -217,6 +217,8 @@ virCommandFDSet(virCommandPtr cmd,
>
> #ifndef WIN32
>
> +static void virDummyHandler(int sig G_GNUC_UNUSED) {}
> +
> /**
> * virFork:
> *
> @@ -312,6 +314,9 @@ virFork(void)
> ignore_value(sigaction(i, &sig_action, NULL));
> }
>
Put a comment here
/* Code that runs between fork & execve might trigger
* SIG_PIPE, so we must explicitly set that to a no-op
* handler. This handler will get reset to SIG_DFL when
* execve() runs
*/
> + sig_action.sa_handler = virDummyHandler;
> + ignore_value(sigaction(SIGPIPE, &sig_action, NULL));
> +
> /* Unmask all signals in child, since we've no idea what the
> * caller's done with their signal mask and don't want to
> * propagate that to children */
> @@ -550,7 +555,7 @@ virExec(virCommandPtr cmd)
> g_autofree char *binarystr = NULL;
> const char *binary = NULL;
> int ret;
> - struct sigaction waxon, waxoff;
> + struct sigaction sig_action;
> g_autofree gid_t *groups = NULL;
> int ngroups;
>
> @@ -718,21 +723,6 @@ virExec(virCommandPtr cmd)
> }
> }
>
> - /* virFork reset all signal handlers to the defaults.
> - * This is good for the child process, but our hook
> - * risks running something that generates SIGPIPE,
> - * so we need to temporarily block that again
> - */
> - memset(&waxoff, 0, sizeof(waxoff));
> - waxoff.sa_handler = SIG_IGN;
> - sigemptyset(&waxoff.sa_mask);
> - memset(&waxon, 0, sizeof(waxon));
> - if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) {
> - virReportSystemError(errno, "%s",
> - _("Could not disable SIGPIPE"));
> - goto fork_error;
> - }
> -
> if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0)
> goto fork_error;
> if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0)
> @@ -783,7 +773,10 @@ virExec(virCommandPtr cmd)
> if (virCommandHandshakeChild(cmd) < 0)
> goto fork_error;
>
> - if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
> + memset(&sig_action, 0, sizeof(sig_action));
> + sig_action.sa_handler = SIG_DFL;
> + sigemptyset(&sig_action.sa_mask);
> + if (sigaction(SIGPIPE, &sig_action, NULL) < 0) {
> virReportSystemError(errno, "%s",
> _("Could not re-enable SIGPIPE"));
> goto fork_error;
We can just delete this sigaction() call entirely. The execve() call
will purge the no-op handler function we registered in virFork(),
resetting it back to SIG_DFL, so no need todo it manually here.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
