11 p.m.
I tried to use 'virsh qemu-attach', and hit different failures
with both libvirt.git on Fedora 19, and with an older version
of libvirt on RHEL 6. I'm posting patches that I have so far
to start reviews, but need to write still more patches as I
still haven't succeeded at attaching. I also need to add a
patch to the testsuite to cover the command line that I'm
trying to attach to.
Eric Blake (3):
qemu: only parse basename when determining emulator properties
qemu: simplify list cleanup
qemu: recognize -machine accel=kvm when parsing native
src/conf/domain_conf.c | 6 ++++
src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++------------------------
2 files changed, 50 insertions(+), 43 deletions(-)
--
1.8.3.1
11 p.m.
New subject: [libvirt] [PATCH 1/3] qemu: only parse basename when determining emulator properties
'virsh domxml-from-native' and 'virsh qemu-attach' could misbehave
for an emulator installed in (a somewhat unlikely) location
such as /usr/local/qemu-1.6/qemu-system-x86_64 or (an even less
likely) /opt/notxen/qemu-system-x86_64. Limit the strstr seach
to just the basename of the file where we are assuming details
about the binary based on its name.
While testing, I accidentally triggered a core dump during strcmp
when I forgot to set os.type on one of my code paths; this patch
changes such a coding error to raise a nicer internal error instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Compute basename
earlier.
* src/conf/domain_conf.c (virDomainDefPostParseInternal): Avoid
NULL deref.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++++++
src/qemu/qemu_command.c | 22 ++++++++++------------
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8a187a6..d356181 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2704,6 +2704,12 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
{
size_t i;
+ if (!def->os.type) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("hypervisor type must be specified"));
+ return -1;
+ }
+
/* verify init path for container based domains */
if (STREQ(def->os.type, "exe") && !def->os.init) {
virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b5ac15a..bc55859 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -28,6 +28,7 @@
#include "qemu_capabilities.h"
#include "qemu_bridge_filter.h"
#include "cpu/cpu.h"
+#include "dirname.h"
#include "passfd.h"
#include "viralloc.h"
#include "virlog.h"
@@ -10764,29 +10765,25 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
if (VIR_STRDUP(def->emulator, progargv[0]) < 0)
goto error;
- if (strstr(def->emulator, "kvm")) {
- def->virtType = VIR_DOMAIN_VIRT_KVM;
- def->features |= (1 << VIR_DOMAIN_FEATURE_PAE);
- }
-
+ if (!(path = last_component(def->emulator)))
+ goto error;
- if (strstr(def->emulator, "xenner")) {
+ if (strstr(path, "xenner")) {
def->virtType = VIR_DOMAIN_VIRT_KVM;
if (VIR_STRDUP(def->os.type, "xen") < 0)
goto error;
} else {
if (VIR_STRDUP(def->os.type, "hvm") < 0)
goto error;
+ if (strstr(path, "kvm")) {
+ def->virtType = VIR_DOMAIN_VIRT_KVM;
+ def->features |= (1 << VIR_DOMAIN_FEATURE_PAE);
+ }
}
- if (STRPREFIX(def->emulator, "qemu"))
- path = def->emulator;
- else
- path = strstr(def->emulator, "qemu");
if (def->virtType == VIR_DOMAIN_VIRT_KVM)
def->os.arch = qemuCaps->host.arch;
- else if (path &&
- STRPREFIX(path, "qemu-system-"))
+ else if (STRPREFIX(path, "qemu-system-"))
def->os.arch = virArchFromString(path + strlen("qemu-system-"));
else
def->os.arch = VIR_ARCH_I686;
@@ -10795,6 +10792,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
(def->os.arch == VIR_ARCH_X86_64))
def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI)
/*| (1 << VIR_DOMAIN_FEATURE_APIC)*/;
+
#define WANT_VALUE() \
const char *val = progargv[++i]; \
if (!val) { \
--
1.8.3.1
7:14 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: only parse basename when determining emulator properties
On 28.08.2013 06:00, Eric Blake wrote:
'virsh domxml-from-native' and 'virsh qemu-attach'
could misbehave
for an emulator installed in (a somewhat unlikely) location
such as /usr/local/qemu-1.6/qemu-system-x86_64 or (an even less
likely) /opt/notxen/qemu-system-x86_64. Limit the strstr seach
to just the basename of the file where we are assuming details
about the binary based on its name.
While testing, I accidentally triggered a core dump during strcmp
when I forgot to set os.type on one of my code paths; this patch
changes such a coding error to raise a nicer internal error instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Compute basename
earlier.
* src/conf/domain_conf.c (virDomainDefPostParseInternal): Avoid
NULL deref.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++++++
src/qemu/qemu_command.c | 22 ++++++++++------------
2 files changed, 16 insertions(+), 12 deletions(-)
ACK
Michal
10:34 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: only parse basename when determining emulator properties
On 09/04/2013 06:14 AM, Michal Privoznik wrote:
On 28.08.2013 06:00, Eric Blake wrote:
> 'virsh domxml-from-native' and 'virsh qemu-attach' could misbehave
> for an emulator installed in (a somewhat unlikely) location
> such as /usr/local/qemu-1.6/qemu-system-x86_64 or (an even less
> likely) /opt/notxen/qemu-system-x86_64. Limit the strstr seach
> to just the basename of the file where we are assuming details
> about the binary based on its name.
>
> While testing, I accidentally triggered a core dump during strcmp
> when I forgot to set os.type on one of my code paths; this patch
> changes such a coding error to raise a nicer internal error instead.
>
> * src/qemu/qemu_command.c (qemuParseCommandLine): Compute basename
> earlier.
> * src/conf/domain_conf.c (virDomainDefPostParseInternal): Avoid
> NULL deref.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> src/conf/domain_conf.c | 6 ++++++
> src/qemu/qemu_command.c | 22 ++++++++++------------
> 2 files changed, 16 insertions(+), 12 deletions(-)
ACK
Thanks; pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: simplify list cleanup
No need to open code now that we have a nice function.
* src/qemu/qemu_command.c (qemuParseCommandLinePid): Simplify cleanup.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_command.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bc55859..6c5f247 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -11777,11 +11777,7 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr qemuCaps,
cleanup:
VIR_FREE(exepath);
- for (i = 0; progargv && progargv[i]; i++)
- VIR_FREE(progargv[i]);
- VIR_FREE(progargv);
- for (i = 0; progenv && progenv[i]; i++)
- VIR_FREE(progenv[i]);
- VIR_FREE(progenv);
+ virStringFreeList(progargv);
+ virSTringFreeList(progenv);
return def;
}
--
1.8.3.1
10:55 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: simplify list cleanup
On 08/27/2013 10:00 PM, Eric Blake wrote:
No need to open code now that we have a nice function.
* src/qemu/qemu_command.c (qemuParseCommandLinePid): Simplify cleanup.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_command.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bc55859..6c5f247 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -11777,11 +11777,7 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr qemuCaps,
cleanup:
VIR_FREE(exepath);
- for (i = 0; progargv && progargv[i]; i++)
- VIR_FREE(progargv[i]);
- VIR_FREE(progargv);
- for (i = 0; progenv && progenv[i]; i++)
- VIR_FREE(progenv[i]);
- VIR_FREE(progenv);
+ virStringFreeList(progargv);
+ virSTringFreeList(progenv);
Self-nack to this version of the patch - this fails to build (STring
instead of String, and even then, I got a complaint about 'char **' and
'const char **' being incomptable).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: recognize -machine accel=kvm when parsing native
In Fedora 19, 'qemu-kvm' is a simple wrapper that calls
'qemu-system-x86_64 -machine accel=kvm'. Attempting
to use 'virsh qemu-attach $pid' to a machine started as:
qemu-kvm -cdrom /var/lib/libvirt/images/foo.img \
-monitor unix:/tmp/demo,server,nowait -name foo \
--uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea
was failing with:
error: XML error: No PCI buses available
because we did not see 'kvm' in the executable name parsed
from /proc/$pid/cmdline, which in turn led to not setting
def->os.machine correctly, which in turn led to refusal to
recognize the pci bus.
Noticed while investigating https://bugzilla.redhat.com/995312
although there are still other issues to fix before that bug
will be completely solved.
I've concluded that the existing parser code for native-to-xml
is a horrendous hodge-podge of ad-hoc approaches; I basically
rewrote the -machine section to be a bit saner.
* src/qemu/qemu_command.c (qemuParseCommandLine): Don't assume
-machine argument is always appropriate for os.machine; set
virtType if accel is present.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c5f247..7dfc499 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -11171,35 +11171,42 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
VIR_FREE(def->name);
} else if (STREQ(arg, "-M") ||
STREQ(arg, "-machine")) {
- char *params;
+ char **list;
+ char *param;
+ size_t j = 0;
+
+ /* -machine [type=]name[,prop[=value][,...]]
+ * Set os.machine only if first parameter lacks '=' or
+ * contains explicit type='...' */
WANT_VALUE();
- params = strchr(val, ',');
- if (params == NULL) {
- if (VIR_STRDUP(def->os.machine, val) < 0)
- goto error;
- } else {
- if (VIR_STRNDUP(def->os.machine, val, params - val) < 0)
+ list = virStringSplit(val, ",", 0);
+ param = list[0];
+
+ if (STRPREFIX(param, "type="))
+ param += strlen("type=");
+ if (!strchr(param, '=')) {
+ if (VIR_STRDUP(def->os.machine, param) < 0) {
+ virStringFreeList(list);
goto error;
-
- while (params++) {
- /* prepared for more "-machine" parameters */
- char *tmp = params;
- params = strchr(params, ',');
-
- if (STRPREFIX(tmp, "dump-guest-core=")) {
- tmp += strlen("dump-guest-core=");
- if (params && VIR_STRNDUP(tmp, tmp, params - tmp) <
0)
- goto error;
- def->mem.dump_core = virDomainMemDumpTypeFromString(tmp);
- if (def->mem.dump_core <= 0)
- def->mem.dump_core = VIR_DOMAIN_MEM_DUMP_DEFAULT;
- if (params)
- VIR_FREE(tmp);
- } else if (STRPREFIX(tmp, "mem-merge=off")) {
- def->mem.nosharepages = true;
- }
+ }
+ j++;
+ }
+
+ /* handle all remaining "-machine" parameters */
+ while ((param = list[j++])) {
+ if (STRPREFIX(param, "dump-guest-core=")) {
+ param += strlen("dump-guest-core=");
+ def->mem.dump_core = virDomainMemDumpTypeFromString(param);
+ if (def->mem.dump_core <= 0)
+ def->mem.dump_core = VIR_DOMAIN_MEM_DUMP_DEFAULT;
+ } else if (STRPREFIX(param, "mem-merge=off")) {
+ def->mem.nosharepages = true;
+ } else if (STRPREFIX(param, "accel=kvm")) {
+ def->virtType = VIR_DOMAIN_VIRT_KVM;
+ def->features |= (1 << VIR_DOMAIN_FEATURE_PAE);
}
}
+ virStringFreeList(list);
} else if (STREQ(arg, "-serial")) {
WANT_VALUE();
if (STRNEQ(val, "none")) {
--
1.8.3.1
7:13 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: recognize -machine accel=kvm when parsing native
On 28.08.2013 06:00, Eric Blake wrote:
In Fedora 19, 'qemu-kvm' is a simple wrapper that calls
'qemu-system-x86_64 -machine accel=kvm'. Attempting
to use 'virsh qemu-attach $pid' to a machine started as:
qemu-kvm -cdrom /var/lib/libvirt/images/foo.img \
-monitor unix:/tmp/demo,server,nowait -name foo \
--uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea
was failing with:
error: XML error: No PCI buses available
because we did not see 'kvm' in the executable name parsed
from /proc/$pid/cmdline, which in turn led to not setting
def->os.machine correctly, which in turn led to refusal to
recognize the pci bus.
Noticed while investigating https://bugzilla.redhat.com/995312
although there are still other issues to fix before that bug
will be completely solved.
I've concluded that the existing parser code for native-to-xml
is a horrendous hodge-podge of ad-hoc approaches; I basically
rewrote the -machine section to be a bit saner.
* src/qemu/qemu_command.c (qemuParseCommandLine): Don't assume
-machine argument is always appropriate for os.machine; set
virtType if accel is present.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 25 deletions(-)
ACK
Michal
10:56 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: recognize -machine accel=kvm when parsing native
On 09/04/2013 06:13 AM, Michal Privoznik wrote:
On 28.08.2013 06:00, Eric Blake wrote:
> In Fedora 19, 'qemu-kvm' is a simple wrapper that calls
> 'qemu-system-x86_64 -machine accel=kvm'. Attempting
> to use 'virsh qemu-attach $pid' to a machine started as:
>
> qemu-kvm -cdrom /var/lib/libvirt/images/foo.img \
> -monitor unix:/tmp/demo,server,nowait -name foo \
> --uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea
>
> was failing with:
> error: XML error: No PCI buses available
>
> because we did not see 'kvm' in the executable name parsed
> from /proc/$pid/cmdline, which in turn led to not setting
> def->os.machine correctly, which in turn led to refusal to
> recognize the pci bus.
>
ACK
Thanks; pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:25 a.m.
On Tue, Aug 27, 2013 at 10:00:27PM -0600, Eric Blake wrote:
I tried to use 'virsh qemu-attach', and hit different
failures
with both libvirt.git on Fedora 19, and with an older version
of libvirt on RHEL 6. I'm posting patches that I have so far
to start reviews, but need to write still more patches as I
still haven't succeeded at attaching. I also need to add a
patch to the testsuite to cover the command line that I'm
trying to attach to.
The single biggest problem with the native-to-xml conversion
is that it is totally clueless about '-device' handling. We
really badly need to add support for that. Ideally we'd
improve things to the extent that it can cover all the args
that we're able to generate in the test suite. We'd then
make it a requirement to have both directions work when adding
new CLI arg support
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:56 p.m.
New subject: [libvirt] [PATCH 4/3] qemu: don't leave vm in table on failure to attach
Failure to attach to a domain during 'virsh qemu-attach' left
the list of domains in an odd state:
$ virsh qemu-attach 4176
error: An error occurred, but the cause is unknown
$ virsh list --all
Id Name State
----------------------------------------------------
2 foo shut off
$ virsh qemu-attach 4176
error: Requested operation is not valid: domain is already active as 'foo'
$ virsh undefine foo
error: Failed to undefine domain foo
error: Requested operation is not valid: cannot undefine transient domain
$ virsh shutdown foo
error: Failed to shutdown domain foo
error: invalid argument: monitor must not be NULL
It all stems from leaving the list of domains unmodified on
the initial failure; we should follow the lead of createXML
which removes vm on failure (the actual initial failure still
needs to be fixed in a later patch, but at least this patch
gets us to the point where we aren't getting stuck with an
unremovable "shut off" transient domain).
* src/qemu/qemu_driver.c (qemuDomainQemuAttach): Match cleanup of
qemuDomainCreateXML.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ed29373..a52c954 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13623,8 +13623,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
+ if (qemuDomainObjEndJob(driver, vm) > 0)
+ qemuDomainRemoveInactive(driver, vm);
+ vm = NULL;
monConfig = NULL;
- goto endjob;
+ goto cleanup;
}
monConfig = NULL;
@@ -13632,7 +13635,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
-endjob:
if (qemuDomainObjEndJob(driver, vm) == 0) {
vm = NULL;
goto cleanup;
--
1.8.3.1
7:13 a.m.
New subject: [libvirt] [PATCH 4/3] qemu: don't leave vm in table on failure to attach
On 28.08.2013 22:56, Eric Blake wrote:
Failure to attach to a domain during 'virsh qemu-attach'
left
the list of domains in an odd state:
$ virsh qemu-attach 4176
error: An error occurred, but the cause is unknown
$ virsh list --all
Id Name State
----------------------------------------------------
2 foo shut off
$ virsh qemu-attach 4176
error: Requested operation is not valid: domain is already active as 'foo'
$ virsh undefine foo
error: Failed to undefine domain foo
error: Requested operation is not valid: cannot undefine transient domain
$ virsh shutdown foo
error: Failed to shutdown domain foo
error: invalid argument: monitor must not be NULL
It all stems from leaving the list of domains unmodified on
the initial failure; we should follow the lead of createXML
which removes vm on failure (the actual initial failure still
needs to be fixed in a later patch, but at least this patch
gets us to the point where we aren't getting stuck with an
unremovable "shut off" transient domain).
* src/qemu/qemu_driver.c (qemuDomainQemuAttach): Match cleanup of
qemuDomainCreateXML.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ed29373..a52c954 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13623,8 +13623,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
+ if (qemuDomainObjEndJob(driver, vm) > 0)
+ qemuDomainRemoveInactive(driver, vm);
+ vm = NULL;
monConfig = NULL;
- goto endjob;
+ goto cleanup;
}
While this part looks okay ...
monConfig = NULL;
@@ -13632,7 +13635,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
-endjob:
if (qemuDomainObjEndJob(driver, vm) == 0) {
vm = NULL;
goto cleanup;
... I am not convinced about this part. I mean, in the context, we
create Domain and store it into @dom. But if qemu process dies
meanwhile, so the qemuDomainObjEndJob() returns zero, the @dom refers to
a stale process. Or am I missing something here? I think these lines
should be swapped.
Michal
noon
New subject: [libvirt] [PATCH 4/3] qemu: don't leave vm in table on failure to attach
On 09/04/2013 06:13 AM, Michal Privoznik wrote:
On 28.08.2013 22:56, Eric Blake wrote:
> Failure to attach to a domain during 'virsh qemu-attach' left
> the list of domains in an odd state:
>
> $ virsh qemu-attach 4176
> error: An error occurred, but the cause is unknown
>
> $ virsh list --all
> Id Name State
> ----------------------------------------------------
> 2 foo shut off
>
> +++ b/src/qemu/qemu_driver.c
> @@ -13623,8 +13623,11 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr
conn,
>
> if (qemuProcessAttach(conn, driver, vm, pid,
> pidfile, monConfig, monJSON) < 0) {
> + if (qemuDomainObjEndJob(driver, vm) > 0)
> + qemuDomainRemoveInactive(driver, vm);
> + vm = NULL;
> monConfig = NULL;
> - goto endjob;
> + goto cleanup;
> }
>
While this part looks okay ...
This change exactly mirros what qemuDomainCreateXML was doing (as
mentioned in the commit message)...
> monConfig = NULL;
> @@ -13632,7 +13635,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
> dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> if (dom) dom->id = vm->def->id;
>
> -endjob:
> if (qemuDomainObjEndJob(driver, vm) == 0) {
> vm = NULL;
> goto cleanup;
>
...which left endjob as an unused label.
... I am not convinced about this part. I mean, in the context, we
create Domain and store it into @dom. But if qemu process dies
meanwhile, so the qemuDomainObjEndJob() returns zero, the @dom refers to
a stale process. Or am I missing something here? I think these lines
should be swapped.
If there is still a bug here, then it is pre-existing, and probably a
similar bug in qemuDomainCreateXML. I'll take another look.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:01 p.m.
New subject: [libvirt] [PATCHv2 2/3] qemu: simplify list cleanup
No need to open code now that we have a nice function.
Interestingly, our virStringFreeList function is typed correctly
(a malloc'd list of malloc'd strings is NOT const, whether at the
point where it is created, or at the point where it is cleand up),
so using it with a 'const char **' argument would require a cast
to keep the compiler. I chose instead to remove const from code
even where we don't modify the argument, just to avoid the need
to cast.
* src/qemu/qemu_command.h (qemuParseCommandLine): Drop declaration.
* src/qemu/qemu_command.c (qemuParseProcFileStrings)
(qemuStringToArgvEnv): Don't force malloc'd result to be const.
(qemuParseCommandLinePid, qemuParseCommandLineString): Simplify
cleanup.
(qemuParseCommandLine, qemuFindEnv): Drop const-correctness to
avoid the need to cast in callers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2: fix compilation, and extend the cleanup
src/qemu/qemu_command.c | 72 +++++++++++++++++++------------------------------
src/qemu/qemu_command.h | 7 -----
2 files changed, 27 insertions(+), 52 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 270a826..cfdfe7e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9528,8 +9528,8 @@ qemuBuildChrDeviceStr(char **deviceStr,
* on space
*/
static int qemuStringToArgvEnv(const char *args,
- const char ***retenv,
- const char ***retargv)
+ char ***retenv,
+ char ***retargv)
{
char **arglist = NULL;
int argcount = 0;
@@ -9538,8 +9538,8 @@ static int qemuStringToArgvEnv(const char *args,
size_t i;
const char *curr = args;
const char *start;
- const char **progenv = NULL;
- const char **progargv = NULL;
+ char **progenv = NULL;
+ char **progargv = NULL;
/* Iterate over string, splitting on sequences of ' ' */
while (curr && *curr != '\0') {
@@ -9620,12 +9620,8 @@ static int qemuStringToArgvEnv(const char *args,
return 0;
error:
- for (i = 0; progenv && progenv[i]; i++)
- VIR_FREE(progenv[i]);
- VIR_FREE(progenv);
- for (i = 0; i < argcount; i++)
- VIR_FREE(arglist[i]);
- VIR_FREE(arglist);
+ virStringFreeList(progenv);
+ virStringFreeList(arglist);
return -1;
}
@@ -9633,7 +9629,7 @@ error:
/*
* Search for a named env variable, and return the value part
*/
-static const char *qemuFindEnv(const char **progenv,
+static const char *qemuFindEnv(char **progenv,
const char *name)
{
size_t i;
@@ -10776,13 +10772,14 @@ qemuParseCommandLineBootDevs(virDomainDefPtr def, const char
*str) {
* virDomainDefPtr representing these settings as closely
* as is practical. This is not an exact science....
*/
-virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
- virDomainXMLOptionPtr xmlopt,
- const char **progenv,
- const char **progargv,
- char **pidfile,
- virDomainChrSourceDefPtr *monConfig,
- bool *monJSON)
+static virDomainDefPtr
+qemuParseCommandLine(virCapsPtr qemuCaps,
+ virDomainXMLOptionPtr xmlopt,
+ char **progenv,
+ char **progargv,
+ char **pidfile,
+ virDomainChrSourceDefPtr *monConfig,
+ bool *monJSON)
{
virDomainDefPtr def;
size_t i;
@@ -11734,10 +11731,9 @@ virDomainDefPtr qemuParseCommandLineString(virCapsPtr qemuCaps,
virDomainChrSourceDefPtr *monConfig,
bool *monJSON)
{
- const char **progenv = NULL;
- const char **progargv = NULL;
+ char **progenv = NULL;
+ char **progargv = NULL;
virDomainDefPtr def = NULL;
- size_t i;
if (qemuStringToArgvEnv(args, &progenv, &progargv) < 0)
goto cleanup;
@@ -11746,13 +11742,8 @@ virDomainDefPtr qemuParseCommandLineString(virCapsPtr qemuCaps,
pidfile, monConfig, monJSON);
cleanup:
- for (i = 0; progargv && progargv[i]; i++)
- VIR_FREE(progargv[i]);
- VIR_FREE(progargv);
-
- for (i = 0; progenv && progenv[i]; i++)
- VIR_FREE(progenv[i]);
- VIR_FREE(progenv);
+ virStringFreeList(progargv);
+ virStringFreeList(progenv);
return def;
}
@@ -11760,7 +11751,7 @@ cleanup:
static int qemuParseProcFileStrings(int pid_value,
const char *name,
- const char ***list)
+ char ***list)
{
char *path = NULL;
int ret = -1;
@@ -11769,7 +11760,6 @@ static int qemuParseProcFileStrings(int pid_value,
char *tmp;
size_t nstr = 0;
char **str = NULL;
- size_t i;
if (virAsprintf(&path, "/proc/%d/%s", pid_value, name) < 0)
goto cleanup;
@@ -11796,14 +11786,11 @@ static int qemuParseProcFileStrings(int pid_value,
str[nstr-1] = NULL;
ret = nstr-1;
- *list = (const char **) str;
+ *list = str;
cleanup:
- if (ret < 0) {
- for (i = 0; str && str[i]; i++)
- VIR_FREE(str[i]);
- VIR_FREE(str);
- }
+ if (ret < 0)
+ virStringFreeList(str);
VIR_FREE(data);
VIR_FREE(path);
return ret;
@@ -11817,11 +11804,10 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr qemuCaps,
bool *monJSON)
{
virDomainDefPtr def = NULL;
- const char **progargv = NULL;
- const char **progenv = NULL;
+ char **progargv = NULL;
+ char **progenv = NULL;
char *exepath = NULL;
char *emulator;
- size_t i;
/* The parser requires /proc/pid, which only exists on platforms
* like Linux where pid_t fits in int. */
@@ -11848,11 +11834,7 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr qemuCaps,
cleanup:
VIR_FREE(exepath);
- for (i = 0; progargv && progargv[i]; i++)
- VIR_FREE(progargv[i]);
- VIR_FREE(progargv);
- for (i = 0; progenv && progenv[i]; i++)
- VIR_FREE(progenv[i]);
- VIR_FREE(progenv);
+ virStringFreeList(progargv);
+ virStringFreeList(progenv);
return def;
}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index a9854a3..2f248eb 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -205,13 +205,6 @@ int qemuNetworkPrepareDevices(virDomainDefPtr def);
* NB: def->name can be NULL upon return and the caller
* *must* decide how to fill in a name in this case
*/
-virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
- virDomainXMLOptionPtr xmlopt,
- const char **progenv,
- const char **progargv,
- char **pidfile,
- virDomainChrSourceDefPtr *monConfig,
- bool *monJSON);
virDomainDefPtr qemuParseCommandLineString(virCapsPtr qemuCaps,
virDomainXMLOptionPtr xmlopt,
const char *args,
--
1.8.3.1
7:14 a.m.
New subject: [libvirt] [PATCHv2 2/3] qemu: simplify list cleanup
On 28.08.2013 23:01, Eric Blake wrote:
No need to open code now that we have a nice function.
Interestingly, our virStringFreeList function is typed correctly
(a malloc'd list of malloc'd strings is NOT const, whether at the
point where it is created, or at the point where it is cleand up),
so using it with a 'const char **' argument would require a cast
to keep the compiler. I chose instead to remove const from code
even where we don't modify the argument, just to avoid the need
to cast.
* src/qemu/qemu_command.h (qemuParseCommandLine): Drop declaration.
* src/qemu/qemu_command.c (qemuParseProcFileStrings)
(qemuStringToArgvEnv): Don't force malloc'd result to be const.
(qemuParseCommandLinePid, qemuParseCommandLineString): Simplify
cleanup.
(qemuParseCommandLine, qemuFindEnv): Drop const-correctness to
avoid the need to cast in callers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2: fix compilation, and extend the cleanup
src/qemu/qemu_command.c | 72 +++++++++++++++++++------------------------------
src/qemu/qemu_command.h | 7 -----
2 files changed, 27 insertions(+), 52 deletions(-)
ACK
Michal
8:54 a.m.
New subject: [libvirt] [PATCHv2 2/3] qemu: simplify list cleanup
On Wed, Aug 28, 2013 at 03:01:23PM -0600, Eric Blake wrote:
No need to open code now that we have a nice function.
Interestingly, our virStringFreeList function is typed correctly
(a malloc'd list of malloc'd strings is NOT const, whether at the
point where it is created, or at the point where it is cleand up),
so using it with a 'const char **' argument would require a cast
to keep the compiler. I chose instead to remove const from code
even where we don't modify the argument, just to avoid the need
to cast.
* src/qemu/qemu_command.h (qemuParseCommandLine): Drop declaration.
* src/qemu/qemu_command.c (qemuParseProcFileStrings)
(qemuStringToArgvEnv): Don't force malloc'd result to be const.
(qemuParseCommandLinePid, qemuParseCommandLineString): Simplify
cleanup.
(qemuParseCommandLine, qemuFindEnv): Drop const-correctness to
avoid the need to cast in callers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2: fix compilation, and extend the cleanup
src/qemu/qemu_command.c | 72 +++++++++++++++++++------------------------------
src/qemu/qemu_command.h | 7 -----
2 files changed, 27 insertions(+), 52 deletions(-)
ACK,
This duplicates part of my patch, but yours has a better commit
message wrt these hunks, so please push this change & i'll rebase
mine.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:43 a.m.
New subject: [libvirt] [PATCHv2 2/3] qemu: simplify list cleanup
On 09/04/2013 07:54 AM, Daniel P. Berrange wrote:
On Wed, Aug 28, 2013 at 03:01:23PM -0600, Eric Blake wrote:
> No need to open code now that we have a nice function.
>
> Interestingly, our virStringFreeList function is typed correctly
> (a malloc'd list of malloc'd strings is NOT const, whether at the
> point where it is created, or at the point where it is cleand up),
Oops, I'm just now noticing my typo (s/cleand/cleaned/), but we're stuck
with it...
ACK,
This duplicates part of my patch, but yours has a better commit
message wrt these hunks, so please push this change & i'll rebase
mine.
...due to the fact that you pushed on my behalf. At any rate, thanks
for tackling this rather than waiting for me to finally get back to this
thread.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:38 p.m.
New subject: [libvirt] [PATCH 5/3] qemu: don't leave shutdown inhibited on attach failure
While debugging a failure of 'virsh qemu-attach', I noticed that
we were leaking the count of active domains on failure. This
means that a libvirtd session that is supposed to quit after
active domains disappear will hang around forever.
* src/qemu/qemu_process.c (qemuProcessAttach): Undo count of
active domains on failure.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
Quite a few latent bugs being uncovered while still drilling
down to the root cause of attaching not working.
src/qemu/qemu_process.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 128618b..6fb4a4f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4353,6 +4353,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
const char *model;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virCapsPtr caps = NULL;
+ bool active = false;
VIR_DEBUG("Beginning VM attach process");
@@ -4378,6 +4379,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (virAtomicIntInc(&driver->nactive) == 1 &&
driver->inhibitCallback)
driver->inhibitCallback(true, driver->inhibitOpaque);
+ active = true;
if (virFileMakePath(cfg->logDir) < 0) {
virReportSystemError(errno,
@@ -4549,9 +4551,12 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
return 0;
cleanup:
- /* We jump here if we failed to start the VM for any reason, or
- * if we failed to initialize the now running VM. kill it off and
- * pretend we never started it */
+ /* We jump here if we failed to attach to the VM for any reason.
+ * Leave the domain running, but pretend we never attempted to
+ * attach to it. */
+ if (active && virAtomicIntDecAndTest(&driver->nactive) &&
+ driver->inhibitCallback)
+ driver->inhibitCallback(false, driver->inhibitOpaque);
VIR_FORCE_CLOSE(logfile);
VIR_FREE(seclabel);
VIR_FREE(sec_managers);
--
1.8.3.1
7:13 a.m.
New subject: [libvirt] [PATCH 5/3] qemu: don't leave shutdown inhibited on attach failure
On 29.08.2013 00:38, Eric Blake wrote:
While debugging a failure of 'virsh qemu-attach', I noticed
that
we were leaking the count of active domains on failure. This
means that a libvirtd session that is supposed to quit after
active domains disappear will hang around forever.
* src/qemu/qemu_process.c (qemuProcessAttach): Undo count of
active domains on failure.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
Quite a few latent bugs being uncovered while still drilling
down to the root cause of attaching not working.
src/qemu/qemu_process.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 128618b..6fb4a4f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4353,6 +4353,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
const char *model;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virCapsPtr caps = NULL;
+ bool active = false;
VIR_DEBUG("Beginning VM attach process");
@@ -4378,6 +4379,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
if (virAtomicIntInc(&driver->nactive) == 1 &&
driver->inhibitCallback)
driver->inhibitCallback(true, driver->inhibitOpaque);
+ active = true;
if (virFileMakePath(cfg->logDir) < 0) {
virReportSystemError(errno,
@@ -4549,9 +4551,12 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
return 0;
cleanup:
I don't like this label being 'cleanup' when in fact it should be
'error'. But that's pre-existing.
- /* We jump here if we failed to start the VM for any reason, or
- * if we failed to initialize the now running VM. kill it off and
- * pretend we never started it */
+ /* We jump here if we failed to attach to the VM for any reason.
+ * Leave the domain running, but pretend we never attempted to
+ * attach to it. */
+ if (active && virAtomicIntDecAndTest(&driver->nactive) &&
+ driver->inhibitCallback)
+ driver->inhibitCallback(false, driver->inhibitOpaque);
VIR_FORCE_CLOSE(logfile);
VIR_FREE(seclabel);
VIR_FREE(sec_managers);
ACK
Michal
12:53 p.m.
New subject: [libvirt] [PATCH 5/3] qemu: don't leave shutdown inhibited on attach failure
On 09/04/2013 06:13 AM, Michal Privoznik wrote:
On 29.08.2013 00:38, Eric Blake wrote:
> While debugging a failure of 'virsh qemu-attach', I noticed that
> we were leaking the count of active domains on failure. This
> means that a libvirtd session that is supposed to quit after
> active domains disappear will hang around forever.
>
> * src/qemu/qemu_process.c (qemuProcessAttach): Undo count of
> active domains on failure.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
>
> @@ -4549,9 +4551,12 @@ int qemuProcessAttach(virConnectPtr conn
ATTRIBUTE_UNUSED,
> return 0;
>
> cleanup:
I don't like this label being 'cleanup' when in fact it should be
'error'. But that's pre-existing.
I fixed that.
ACK
and pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4098
days inactive
4107
days old
19 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik