[libvirt] [PATCH 0/n] improvements to native-to-xml parsing
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
'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@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
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@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ src/qemu/qemu_command.c | 22 ++++++++++------------ 2 files changed, 16 insertions(+), 12 deletions(-)
ACK Michal
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@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
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@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
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@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
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@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
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@redhat.com> --- src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 25 deletions(-)
ACK Michal
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
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 :|
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@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
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@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
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
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@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
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@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
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@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 :|
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
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@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
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@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
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@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
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik