[libvirt] [PATCH] Ensure virExec preserves logging environment
The virFork call resets all logging handlers that may have been set. Re-enable them after fork in virExec, so that env variables fir LIBVIRT_LOG_OUTPUTS and LIBVIRT_LOG_FILTERS take effect until the execve() * src/util/util.c: Preserve logging in child in virExec --- src/util/util.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index f2fe58a..a0849e1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -602,6 +602,9 @@ __virExec(const char *const*argv, childout = -1; } + /* Initialize full logging for a while */ + virLogSetFromEnv(); + /* Daemonize as late as possible, so the parent process can detect * the above errors with wait* */ if (flags & VIR_EXEC_DAEMON) { @@ -650,6 +653,9 @@ __virExec(const char *const*argv, virClearCapabilities() < 0) goto fork_error; + /* Close logging again to ensure no FDs leak to child */ + virLogReset(); + if (envp) execve(argv[0], (char **) argv, (char**)envp); else -- 1.7.2.3
On 11/22/2010 07:11 AM, Daniel P. Berrange wrote:
The virFork call resets all logging handlers that may have been set. Re-enable them after fork in virExec, so that env variables fir LIBVIRT_LOG_OUTPUTS and LIBVIRT_LOG_FILTERS take effect until the execve()
* src/util/util.c: Preserve logging in child in virExec --- src/util/util.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index f2fe58a..a0849e1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -602,6 +602,9 @@ __virExec(const char *const*argv, childout = -1; }
+ /* Initialize full logging for a while */ + virLogSetFromEnv();
Should this function be changed to take a bool parameter for marking logging fd's as cloexec? Or, for that matter, should all logging fd's always be cloexec in the first place?
+ /* Daemonize as late as possible, so the parent process can detect * the above errors with wait* */ if (flags & VIR_EXEC_DAEMON) { @@ -650,6 +653,9 @@ __virExec(const char *const*argv, virClearCapabilities() < 0) goto fork_error;
+ /* Close logging again to ensure no FDs leak to child */ + virLogReset();
In which case we wouldn't need this, because the fds would then automatically avoid leaking? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Nov 22, 2010 at 12:01:00PM -0700, Eric Blake wrote:
On 11/22/2010 07:11 AM, Daniel P. Berrange wrote:
The virFork call resets all logging handlers that may have been set. Re-enable them after fork in virExec, so that env variables fir LIBVIRT_LOG_OUTPUTS and LIBVIRT_LOG_FILTERS take effect until the execve()
* src/util/util.c: Preserve logging in child in virExec --- src/util/util.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index f2fe58a..a0849e1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -602,6 +602,9 @@ __virExec(const char *const*argv, childout = -1; }
+ /* Initialize full logging for a while */ + virLogSetFromEnv();
Should this function be changed to take a bool parameter for marking logging fd's as cloexec? Or, for that matter, should all logging fd's always be cloexec in the first place?
Yes, everything libvirt does should be O_CLOEXEC really.
+ /* Daemonize as late as possible, so the parent process can detect * the above errors with wait* */ if (flags & VIR_EXEC_DAEMON) { @@ -650,6 +653,9 @@ __virExec(const char *const*argv, virClearCapabilities() < 0) goto fork_error;
+ /* Close logging again to ensure no FDs leak to child */ + virLogReset();
In which case we wouldn't need this, because the fds would then automatically avoid leaking?
I prefer to be paranoid about closing FDs even if we believe they are all O_CLOEXEC already. Daniel
On 11/23/2010 03:24 AM, Daniel P. Berrange wrote:
Should this function be changed to take a bool parameter for marking logging fd's as cloexec? Or, for that matter, should all logging fd's always be cloexec in the first place?
Yes, everything libvirt does should be O_CLOEXEC really.
Agreed, but that means I need to revive some of my gnulib work to add O_CLOEXEC to open(), as well as import pipe2() and other CLOEXEC-support functions into libvirt, to make it faster on modern kernels. That can be set aside for another day (post 0.8.6).
In which case we wouldn't need this, because the fds would then automatically avoid leaking?
I prefer to be paranoid about closing FDs even if we believe they are all O_CLOEXEC already.
Fair enough. At any rate, your patch looks fine as is; ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake