1:54 p.m.
[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.
1:54 p.m.
New subject: [libvirt] [PATCH v1 1/2] command: Move environ-adding code to common function virCommandAddEnv.
From: "Richard W.M. Jones" <rjones(a)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
2:45 p.m.
New subject: [libvirt] [PATCH v1 1/2] command: Move environ-adding code to common function virCommandAddEnv.
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:54 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
From: "Richard W.M. Jones" <rjones(a)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
2:49 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:53 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
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(a)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
3:07 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:16 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
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
3:24 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:12 a.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
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
5:55 p.m.
New subject: [libvirt] [PATCH v1 2/2] command: Change virCommandAddEnv so it replaces existing environment variables.
On 09/24/2012 12:54 PM, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:43 p.m.
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]
https://bugzilla.redhat.com/show_bug.cgi?id=859596
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4477
days inactive
4478
days old
11 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones