[libvirt] [PATCH v1 0/2] Fix environment-setting code.
[This patch is for discussion only: I have not proven that it fixes the bug] https://bugzilla.redhat.com/show_bug.cgi?id=859596 When libvirtd runs any command, it uses a simple strategy to set the environment for that command: (a) It creates a new, empty environment array. (b) It copies in a few known-good environment variables, including TMPDIR. (c) It appends environment variables specified by the user, eg. ones specified in <qemu:env>. Unfortunately this means if you try to set TMPDIR via a <qemu:env> XML directive, then it is appended to the list of environment variables which already includes TMPDIR. The qemu subprocess picks an environment variable (probably) first from the list, which is the TMPDIR that libvirtd had originally, not the TMPDIR that was specified by the user. This [possibly, not proven] leads to bug 859596, where we see qemu trying to create temporary files in a directory that no longer exists. The follow-up patches change libvirtd so that environment variables in the virCommandPtr array are replaced if they exist already. Rich.
From: "Richard W.M. Jones" <rjones@redhat.com> This is just code motion. The semantics of the code should be identical after this change. --- src/util/command.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 5d6e17b..f7d92dd 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -984,6 +984,22 @@ virCommandNonblockingFDs(virCommandPtr cmd) cmd->flags |= VIR_EXEC_NONBLOCK; } +/* Add an environment variable to the cmd->env list. 'env' is a + * string like "name=value". + */ +static inline void +virCommandAddEnv(virCommandPtr cmd, char *env) +{ + /* Arg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + VIR_FREE(env); + cmd->has_error = ENOMEM; + return; + } + + cmd->env[cmd->nenv++] = env; +} + /** * virCommandAddEnvFormat: * @cmd: the command to modify @@ -1009,14 +1025,7 @@ virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) } va_end(list); - /* Arg plus trailing NULL. */ - if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { - VIR_FREE(env); - cmd->has_error = ENOMEM; - return; - } - - cmd->env[cmd->nenv++] = env; + virCommandAddEnv(cmd, env); } /** @@ -1056,14 +1065,7 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) return; } - /* env plus trailing NULL */ - if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { - VIR_FREE(env); - cmd->has_error = ENOMEM; - return; - } - - cmd->env[cmd->nenv++] = env; + virCommandAddEnv(cmd, env); } @@ -1084,9 +1086,7 @@ virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) return; } - /* env plus trailing NULL. */ - if (virBufferError(buf) || - VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + if (virBufferError(buf)) { cmd->has_error = ENOMEM; virBufferFreeAndReset(buf); return; @@ -1096,7 +1096,7 @@ virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) return; } - cmd->env[cmd->nenv++] = virBufferContentAndReset(buf); + virCommandAddEnv(cmd, virBufferContentAndReset(buf)); } -- 1.7.10.4
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This is just code motion. The semantics of the code should be identical after this change. --- src/util/command.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Richard W.M. Jones" <rjones@redhat.com> --- src/util/command.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/util/command.c b/src/util/command.c index f7d92dd..354e526 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -985,11 +985,26 @@ virCommandNonblockingFDs(virCommandPtr cmd) } /* Add an environment variable to the cmd->env list. 'env' is a - * string like "name=value". + * string like "name=value". If the named environment variable is + * already set, then it is replaced in the list. */ static inline void virCommandAddEnv(virCommandPtr cmd, char *env) { + size_t namelen; + size_t i; + + /* Search for the name in the existing environment. */ + namelen = strcspn(env, "="); + for (i = 0; i < cmd->nenv; ++i) { + /* +1 because we want to match the '=' character too. */ + if (STREQLEN(cmd->env[i], env, namelen+1)) { + VIR_FREE(cmd->env[i]); + cmd->env[i] = env; + return; + } + } + /* Arg plus trailing NULL. */ if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { VIR_FREE(env); -- 1.7.10.4
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
--- src/util/command.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/command.c b/src/util/command.c index f7d92dd..354e526 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -985,11 +985,26 @@ virCommandNonblockingFDs(virCommandPtr cmd) }
/* Add an environment variable to the cmd->env list. 'env' is a - * string like "name=value". + * string like "name=value". If the named environment variable is + * already set, then it is replaced in the list. */ static inline void virCommandAddEnv(virCommandPtr cmd, char *env) { + size_t namelen; + size_t i; + + /* Search for the name in the existing environment. */ + namelen = strcspn(env, "=");
Would 'strchr(env, '=') - env' be any more efficient? But that's a micro-optimization, probably not worth worrying about.
+ for (i = 0; i < cmd->nenv; ++i) { + /* +1 because we want to match the '=' character too. */ + if (STREQLEN(cmd->env[i], env, namelen+1)) {
Coding style - spaces on both sides of '+'. Or even hoist the +1 outside of the loop to the original computation of namelen. ACK with that fix. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Sep 24, 2012 at 01:49:37PM -0600, Eric Blake wrote:
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
--- src/util/command.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/command.c b/src/util/command.c index f7d92dd..354e526 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -985,11 +985,26 @@ virCommandNonblockingFDs(virCommandPtr cmd) }
/* Add an environment variable to the cmd->env list. 'env' is a - * string like "name=value". + * string like "name=value". If the named environment variable is + * already set, then it is replaced in the list. */ static inline void virCommandAddEnv(virCommandPtr cmd, char *env) { + size_t namelen; + size_t i; + + /* Search for the name in the existing environment. */ + namelen = strcspn(env, "=");
Would 'strchr(env, '=') - env' be any more efficient? But that's a micro-optimization, probably not worth worrying about.
I guess I trust glibc or gcc to have these string primitives optimized better than I could.
+ for (i = 0; i < cmd->nenv; ++i) { + /* +1 because we want to match the '=' character too. */ + if (STREQLEN(cmd->env[i], env, namelen+1)) {
Coding style - spaces on both sides of '+'. Or even hoist the +1 outside of the loop to the original computation of namelen.
ACK with that fix.
Thanks -- both patches pushed, the second one with the recommended change. I will continue with testing to see if this does or doesn't fix the actual bug. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 09/24/2012 01:53 PM, Richard W.M. Jones wrote:
+ /* Search for the name in the existing environment. */ + namelen = strcspn(env, "=");
Would 'strchr(env, '=') - env' be any more efficient? But that's a micro-optimization, probably not worth worrying about.
I guess I trust glibc or gcc to have these string primitives optimized better than I could.
Ah, but glibc is open source, so we can check for ourselves: The naive C fallback when no .S is present is highly unoptimized. From glibcc/string/strcspn.c: size_t strcspn (s, reject) const char *s; const char *reject; { size_t count = 0; while (*s != '\0') if (strchr (reject, *s++) == NULL) ++count; else return count; return count; } and even in the .S optimized versions, there's still no shortcuts taken for a one-character reject (possibly worth filing a BZ about the missed optimization, though). From glibc/sysdeps/x86_64/strcspn.S: /* First we create a table with flags for all possible characters. For the ASCII (7bit/8bit) or ISO-8859-X character sets which are supported by the C string functions we have 256 characters. Before inserting marks for the stop characters we clear the whole table. */ movq %rdi, %r8 /* Save value. */ subq $256, %rsp /* Make space for 256 bytes. */ cfi_adjust_cfa_offset(256) movl $32, %ecx /* 32*8 bytes = 256 bytes. */ movq %rsp, %rdi xorl %eax, %eax /* We store 0s. */ ... That is, you are definitely wasting time pre-computing the reject table, compared to doing a strchr() for the one rejection. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Sep 24, 2012 at 02:07:36PM -0600, Eric Blake wrote:
On 09/24/2012 01:53 PM, Richard W.M. Jones wrote:
+ /* Search for the name in the existing environment. */ + namelen = strcspn(env, "=");
Would 'strchr(env, '=') - env' be any more efficient? But that's a micro-optimization, probably not worth worrying about.
I guess I trust glibc or gcc to have these string primitives optimized better than I could.
Ah, but glibc is open source, so we can check for ourselves:
The naive C fallback when no .S is present is highly unoptimized. From glibcc/string/strcspn.c:
size_t strcspn (s, reject) const char *s; const char *reject; { size_t count = 0;
while (*s != '\0') if (strchr (reject, *s++) == NULL) ++count; else return count;
return count; }
and even in the .S optimized versions, there's still no shortcuts taken for a one-character reject (possibly worth filing a BZ about the missed optimization, though). From glibc/sysdeps/x86_64/strcspn.S: /* First we create a table with flags for all possible characters. For the ASCII (7bit/8bit) or ISO-8859-X character sets which are supported by the C string functions we have 256 characters. Before inserting marks for the stop characters we clear the whole table. */ movq %rdi, %r8 /* Save value. */ subq $256, %rsp /* Make space for 256 bytes. */ cfi_adjust_cfa_offset(256) movl $32, %ecx /* 32*8 bytes = 256 bytes. */ movq %rsp, %rdi xorl %eax, %eax /* We store 0s. */ ...
That is, you are definitely wasting time pre-computing the reject table, compared to doing a strchr() for the one rejection.
Possibly, if we weren't just about to spend 100000 cycles doing a fork. Now, a project to rewrite all Python utilities in Linux in a compiled high-level language like OCaml, there's something I could get behind :-) -*- Unfortunately this patch does not fix the bug, but it now fails in a different, and stranger way: libvir: error : libvirtd quit during handshake: Input/output error In this code: if ((rv = saferead(cmd->handshakeNotify[0], &c, sizeof(c))) != sizeof(c)) { if (rv < 0) virReportSystemError(errno, "%s", _("Unable to wait on parent process")); else virReportSystemError(EIO, "%s", _("libvirtd quit during handshake")); return -1; } Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 09/24/2012 02:16 PM, Richard W.M. Jones wrote:
On Mon, Sep 24, 2012 at 02:07:36PM -0600, Eric Blake wrote:
On 09/24/2012 01:53 PM, Richard W.M. Jones wrote:
+ /* Search for the name in the existing environment. */ + namelen = strcspn(env, "=");
Would 'strchr(env, '=') - env' be any more efficient? But that's a micro-optimization, probably not worth worrying about.
I guess I trust glibc or gcc to have these string primitives optimized better than I could.
Ah, but glibc is open source, so we can check for ourselves:
That is, you are definitely wasting time pre-computing the reject table, compared to doing a strchr() for the one rejection.
http://sourceware.org/bugzilla/show_bug.cgi?id=14613
Possibly, if we weren't just about to spend 100000 cycles doing a fork. Now, a project to rewrite all Python utilities in Linux in a compiled high-level language like OCaml, there's something I could get behind :-)
Precisely why I said it is a 'micro-optimization, not worth worrying about in this context' :)
Unfortunately this patch does not fix the bug, but it now fails in a different, and stranger way:
libvir: error : libvirtd quit during handshake: Input/output error
In this code:
if ((rv = saferead(cmd->handshakeNotify[0], &c, sizeof(c))) != sizeof(c)) { if (rv < 0) virReportSystemError(errno, "%s", _("Unable to wait on parent process")); else virReportSystemError(EIO, "%s", _("libvirtd quit during handshake")); return -1; }
Ewww. Something went wrong in the child, which in turn messed up the handshake notifier. Can you set up gdb to follow the child after fork, and see why the child is going haywire? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Sep 24, 2012 at 02:24:23PM -0600, Eric Blake wrote:
On 09/24/2012 02:16 PM, Richard W.M. Jones wrote:
Unfortunately this patch does not fix the bug, but it now fails in a different, and stranger way:
libvir: error : libvirtd quit during handshake: Input/output error
In this code:
if ((rv = saferead(cmd->handshakeNotify[0], &c, sizeof(c))) != sizeof(c)) { if (rv < 0) virReportSystemError(errno, "%s", _("Unable to wait on parent process")); else virReportSystemError(EIO, "%s", _("libvirtd quit during handshake")); return -1; }
Ewww. Something went wrong in the child, which in turn messed up the handshake notifier.
This turned out to be an SELinux policy problem stopping qemu from creating temporary files, which was exposed only once we started setting $TMPDIR correctly for qemu. https://bugzilla.redhat.com/show_bug.cgi?id=860235 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
--- src/util/command.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Any new feature deserves tests; I'm pushing this under the trivial rule. diff --git c/tests/commandtest.c w/tests/commandtest.c index 930df5c..cba6cb6 100644 --- c/tests/commandtest.c +++ w/tests/commandtest.c @@ -331,7 +331,9 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + virCommandAddEnvString(cmd, "USER=bogus"); virCommandAddEnvString(cmd, "LANG=C"); + virCommandAddEnvPair(cmd, "USER", "also bogus"); virCommandAddEnvPair(cmd, "USER", "test"); if (virCommandRun(cmd, NULL) < 0) { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
[This patch is for discussion only: I have not proven that it fixes the bug]
Even if it doesn't fix the bug...
The follow-up patches change libvirtd so that environment variables in the virCommandPtr array are replaced if they exist already.
...this change still sounds useful, so I'll go ahead and review them. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones