Re: [libvirt] [PATCH] qemu: Always enable the virtio balloon driver
On Tue, Jan 05, 2010 at 09:43:32AM +0200, Dor Laor wrote: On 01/04/2010 07:13 PM, Adam Litke wrote: The behavior for the qemu balloon device has changed. Formerly, a virtio balloon device was provided by default. Now, '-balloon virtio' must be specified on the command line to enable it. This patch causes libvirt to add '-balloon virtio' to the command line whenever the -balloon option is available. Why add it automatically? I rather have the user state explicitly that a balloon is required. If the balloon is not used, we're just wasting a pci slot and enlarge the hypervisor signature. I'm in favor of using the balloon driver, but there are occasions it won't be required or we won't have a guest driver for it. Not having the balloon driver present is a regression from earlier libvirt behaviour, and likewise requiring it to be explicitly added to a guest will cause an immediate regression for all apps using libvirt Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Always enable the virtio balloon driver
On Mon, Jan 04, 2010 at 11:13:04AM -0600, Adam Litke wrote:
The behavior for the qemu balloon device has changed. Formerly, a virtio
balloon device was provided by default. Now, '-balloon virtio' must be
specified on the command line to enable it. This patch causes libvirt to add
'-balloon virtio' to the command line whenever the -balloon option is
available.
Signed-off-by: Adam Litke a...@us.ibm.com
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 36bf9a2..07b0bd1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1113,6 +1113,8 @@ static unsigned int qemudComputeCmdFlags(const char
*help,
flags |= QEMUD_CMD_FLAG_MEM_PATH;
if (strstr(help, -chardev))
flags |= QEMUD_CMD_FLAG_CHARDEV;
+if (strstr(help, -balloon))
+flags |= QEMUD_CMD_FLAG_BALLOON;
if (version = 9000)
flags |= QEMUD_CMD_FLAG_VNC_COLON;
@@ -2883,6 +2885,14 @@ int qemudBuildCommandLine(virConnectPtr conn,
ADD_ARG_LIT(migrateFrom);
}
+/* QEMU changed its default behavior to not include the virtio balloon
+ * device. Explicitly request it to ensure it will be present.
+ */
+if (qemuCmdFlags QEMUD_CMD_FLAG_BALLOON) {
+ADD_ARG_LIT(-balloon);
+ADD_ARG_LIT(virtio);
+}
+
ADD_ARG(NULL);
ADD_ENV(NULL);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index e958850..28f59bf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -78,6 +78,7 @@ enum qemud_cmd_flags {
QEMUD_CMD_FLAG_ENABLE_KVM= (1 23), /* Is the -enable-kvm flag
available to enable KVM full virtualization support */
QEMUD_CMD_FLAG_0_12 = (1 24),
QEMUD_CMD_FLAG_MONITOR_JSON = QEMUD_CMD_FLAG_0_12, /* JSON mode for
monitor */
+QEMUD_CMD_FLAG_BALLOON = (1 25), /* -balloon available */
};
/* Main driver state */
ACK, this looks good to me.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix parsing of 'info chardev' line endings
On Mon, Dec 14, 2009 at 03:31:12PM +, Matthew Booth wrote:
This change makes the 'info chardev' parser ignore any trailing whitespace on
a
line. This fixes a specific problem handling a '\r\n' line ending.
* src/qemu/qemu_monitor_text.c: Ignore trailing whitespace in 'info chardev'
output.
---
src/qemu/qemu_monitor_text.c | 26 +-
1 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 0cb9ea6..4fd8c4a 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1622,15 +1622,26 @@ int qemuMonitorTextGetPtyPaths(qemuMonitorPtr mon,
goto cleanup;
}
-char *pos = reply; /* The current start of searching */
-char *end = pos + strlen(reply);/* The end of the reply string */
+char *pos; /* The current start of searching */
+char *next = reply; /* The start of the next line */
char *eol; /* The character which ends the current
line */
+char *end = reply + strlen(reply); /* The end of the reply string */
+
+while (next) {
+pos = next;
-while (pos end) {
/* Split the output into lines */
eol = memchr(pos, '\n', end - pos);
-if (eol == NULL)
+if (eol == NULL) {
eol = end;
+next = NULL;
+} else {
+next = eol + 1;
+}
+
+/* Ignore all whitespace immediately before eol */
+while (eol pos c_isspace(*(eol-1)))
+eol -= 1;
/* Look for 'filename=pty:' */
#define NEEDLE filename=pty:
@@ -1638,13 +1649,13 @@ int qemuMonitorTextGetPtyPaths(qemuMonitorPtr mon,
/* If it's not there we can ignore this line */
if (!needle)
-goto next;
+continue;
/* id is everthing from the beginning of the line to the ':'
* find ':' and turn it into a terminator */
char *colon = memchr(pos, ':', needle - pos);
if (colon == NULL)
-goto next;
+continue;
*colon = '\0';
char *id = pos;
@@ -1664,9 +1675,6 @@ int qemuMonitorTextGetPtyPaths(qemuMonitorPtr mon,
goto cleanup;
}
#undef NEEDLE
-
-next:
-pos = eol + 1;
}
ret = 0;
ACK, seems this patch got missed which is unfortunate, since it results
in XML containining \r in an attribute
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] allow (only) surrounding whitespace in uuid
Please consider something along these lines. Without it, pretty-printed
domxml is rejected due to the whitespace before uuid, and long long
string of hexadecimal digits is accepted.
---
src/util/uuid.c | 12 +++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 002a64d..0f2ca96 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -145,9 +145,13 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) {
/*
* do a liberal scan allowing '-' and ' ' anywhere between character
- * pairs as long as there is 32 of them in the end.
+ * pairs, and surrounding whitespace, as long as there are exactly
+ * 32 hexadecimal digits the end.
*/
cur = uuidstr;
+while (c_isspace(*cur))
+cur++;
+
for (i = 0;i VIR_UUID_BUFLEN;) {
uuid[i] = 0;
if (*cur == 0)
@@ -170,6 +174,12 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) {
cur++;
}
+while (*cur) {
+if (!c_isspace(*cur))
+goto error;
+cur++;
+}
+
return 0;
error:
--
1.6.5.2
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Update qemuhelpdata test with new flag
The test suite needs a small fixup with the qemu balloon patch applied. This patch applies on top of: qemu: Always enable the virtio balloon driver A small update to the qemuhelpdata test is required. It must be told about the new QEMUD_CMD_FLAG_BALLOON flag that will be turned on for qemu-kvm-0.11.0-rc2. Signed-off-by: Adam Litke a...@us.ibm.com diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index a747da7..0a78b05 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -179,7 +179,8 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_0_10 | QEMUD_CMD_FLAG_PCIDEVICE | QEMUD_CMD_FLAG_MEM_PATH | -QEMUD_CMD_FLAG_ENABLE_KVM, +QEMUD_CMD_FLAG_ENABLE_KVM | +QEMUD_CMD_FLAG_BALLOON, 10092, 1, 0); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- Thanks, Adam -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] openvz_conf.c: don't dereference NULL upon failure
Daniel P. Berrange wrote:
On Tue, Dec 15, 2009 at 04:19:23PM +0100, Jim Meyering wrote:
dom is set to NULL within the while loop:
virDomainObjUnlock(dom);
dom = NULL;
If on a subsequent iteration something fails,
we goto cleanup or no_memory, both of which
have us run this code:
fclose(fp);
virDomainObjUnref(dom);
return -1;
And the virDomainObjUnref function would dereference dom.
From 3971ff17c7e9f1ddbc443d48b86fe6ba60a2d4a0 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 15 Dec 2009 16:16:57 +0100
Subject: [PATCH] openvz_conf.c: don't dereference NULL upon failure
* src/openvz/openvz_conf.c (openvzLoadDomains): Avoid NULL deref
of dom.
---
src/openvz/openvz_conf.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 7e9abbf..43bbaf2 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -535,7 +535,8 @@ int openvzLoadDomains(struct openvz_driver *driver) {
cleanup:
fclose(fp);
-virDomainObjUnref(dom);
+if (dom)
+virDomainObjUnref(dom);
return -1;
}
--
ACK
Pushed.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network/bridge_driver.c: avoid potential NULL-dereference
obvious typo:
From 6f0810192dc6d9b223f2f973812fd787f398576c Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 5 Jan 2010 15:48:42 +0100
Subject: [PATCH] network/bridge_driver.c: avoid potential NULL-dereference
* src/network/bridge_driver.c (networkBuildDnsmasqArgv): Correct
test for NULL *argv.
---
src/network/bridge_driver.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index abee78c..69809b3 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1,7 +1,7 @@
/*
* driver.c: core driver methods for managing qemu guests
*
- * Copyright (C) 2006-2009 Red Hat, Inc.
+ * Copyright (C) 2006-2010 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -513,7 +513,7 @@ networkBuildDnsmasqArgv(virConnectPtr conn,
return 0;
no_memory:
-if (argv) {
+if (*argv) {
for (i = 0; (*argv)[i]; i++)
VIR_FREE((*argv)[i]);
VIR_FREE(*argv);
--
1.6.6.384.g14e6a
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu_driver.c: remove useless, warning-provoking test
Tools like coverity and maybe clang notice that we dereference uri_out, and then (later) test whether uri_out is NULL. Obviously one or the other must go. Presuming that we will always pass a non-NULL uri_out arg to this function, I chose to mark that parameter as ATTRIBUTE_NONNULL. I removed the unnecessary test, too. If anyone is concerned that a non-NULL (non-literal) value will be passed, I can also add an assertion. From 6ce3a9656d978b37d866f55606bc5031ad8e6b2f Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 5 Jan 2010 16:32:11 +0100 Subject: [PATCH] qemu_driver.c: remove useless, warning-provoking test * src/qemu/qemu_driver.c (qemudDomainMigratePrepare2): Remove useless test of always-non-NULL uri_out parameter. Use ATTRIBUTE_NONNULL to inform tools. --- src/qemu/qemu_driver.c |6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4639478..daa6f94 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,7 @@ /* * driver.c: core driver methods for managing qemu guests * - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009, 2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -6961,7 +6961,7 @@ cleanup: * * This starts an empty VM listening on a TCP port. */ -static int +static int ATTRIBUTE_NONNULL (5) qemudDomainMigratePrepare2 (virConnectPtr dconn, char **cookie ATTRIBUTE_UNUSED, int *cookielen ATTRIBUTE_UNUSED, @@ -7068,7 +7068,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, } } -if (uri_out *uri_out) +if (*uri_out) VIR_DEBUG(Generated uri_out=%s, *uri_out); /* Parse the domain XML. */ -- 1.6.6.387.g2649b1 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Migration with non-shared storage
Support for live migration between hosts that do not share storage was added to qemu-kvm release 0.12.1. It supports two flags: -b migration without shared storage with full disk copy -i migration without shared storage with incremental copy (same base image shared between source and destination). I suggest adding these flags to virDomainMigrate. If I'm not mistaken qemuMonitorTextMigrate is the function that actually invokes the kvm monitor. Thus, it would be necessary to pass the flags to qemuMonitorTextMigrate.. But qemuMonitorTextMigrate does not have a flag input parameter. I think the least disruptive way to support the new flags is use the existing background parameter to pass the flags. Of course this would require some changes to the upstream functions that are invoked for migration. What do you think? Kenneth Nagin -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_driver.c: remove useless, warning-provoking test
On 01/05/2010 10:37 AM, Jim Meyering wrote: Tools like coverity and maybe clang notice that we dereference uri_out, and then (later) test whether uri_out is NULL. Obviously one or the other must go. Presuming that we will always pass a non-NULL uri_out arg to this function, I chose to mark that parameter as ATTRIBUTE_NONNULL. I removed the unnecessary test, too. If anyone is concerned that a non-NULL (non-literal) value will be passed, I can also add an assertion. The change you make below is correct. uri_out can never be NULL in the code we have today, so the additional check is useless. ACK -- Chris Lalancette -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_driver.c: avoid NULL dereference upon disk-op failure
Matthias Bolte wrote:
2009/12/16 Jim Meyering j...@meyering.net:
The local, cgroup is initialized to NULL, and may still
have that value when the code below is reached.
That would provoke a NULL-dereference in virCgroupDenyDevicePath.
From c1caed370a7b2eae2e964a6059b014530143075c Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 16 Dec 2009 14:15:50 +0100
Subject: [PATCH] qemu_driver.c: avoid NULL dereference upon disk-op failure
* src/qemu/qemu_driver.c (qemudDomainAttachDevice): Call
virCgroupDenyDevicePath only if cgroup is non-NULL.
---
src/qemu/qemu_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9ef6c35..81afecf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5485,7 +5485,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
virDomainDiskDeviceTypeToString(dev-data.disk-device));
/* Fallthrough */
}
- if (ret != 0) {
+ if (ret != 0 cgroup) {
virCgroupDenyDevicePath(cgroup,
dev-data.disk-src);
}
--
1.6.6.rc2.275.g51e2d
ACK.
Thanks. Pushed.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_driver.c: remove useless, warning-provoking test
Chris Lalancette wrote: On 01/05/2010 10:37 AM, Jim Meyering wrote: Tools like coverity and maybe clang notice that we dereference uri_out, and then (later) test whether uri_out is NULL. Obviously one or the other must go. Presuming that we will always pass a non-NULL uri_out arg to this function, I chose to mark that parameter as ATTRIBUTE_NONNULL. I removed the unnecessary test, too. If anyone is concerned that a non-NULL (non-literal) value will be passed, I can also add an assertion. The change you make below is correct. uri_out can never be NULL in the code we have today, so the additional check is useless. ACK Thanks for the review. Pushed. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
Matthias Bolte wrote:
2009/12/16 Jim Meyering j...@meyering.net:
Similar to others,
From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 16 Dec 2009 13:56:57 +0100
Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML
failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call
vboxDomainUndefine on a NULL dom.
---
src/vbox/vbox_tmpl.c | 16 +---
1 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index d6b681c..ba70053 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml,
unsigned int flags ATTRIBUTE_UNUSED)
{
- virDomainPtr dom = NULL;
-
/* VirtualBox currently doesn't have support for running
* virtual machines without actually defining them and thus
* for time being just define new machine and start it.
@@ -1000,17 +998,13 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr
conn, const char *xml,
* change this behaviour to the expected one.
*/
- dom = vboxDomainDefineXML(conn, xml);
- if (dom) {
- if (vboxDomainCreate(dom) 0)
- goto cleanup;
- } else {
- goto cleanup;
- }
+ virDomainPtr dom = vboxDomainDefineXML(conn, xml);
+ if (dom == NULL)
+ return NULL;
- return dom;
+ if (0 vboxDomainCreate(dom))
+ return dom;
This check is wrong. You meant to check for !(vboxDomainCreate(dom)
0) but resolved the ! incorrectly.
I reversed the condition, but left off the =.
Either change it to
if (vboxDomainCreate(dom) = 0)
return dom;
or keep the negative check
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
return NULL;
}
return dom;
I prefer the second version.
So do I.
Here's the adjusted patch.
From 7e8355ea7c2e50995b139322de574ea4abf24fe3 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 16 Dec 2009 13:56:57 +0100
Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call
vboxDomainUndefine on a NULL dom.
---
src/vbox/vbox_tmpl.c | 19 +++
1 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 5a1d8dc..5889f32 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml,
unsigned int flags ATTRIBUTE_UNUSED) {
-virDomainPtr dom = NULL;
-
/* VirtualBox currently doesn't have support for running
* virtual machines without actually defining them and thus
* for time being just define new machine and start it.
@@ -1000,19 +998,16 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr
conn, const char *xml,
* change this behaviour to the expected one.
*/
-dom = vboxDomainDefineXML(conn, xml);
-if (dom) {
-if (vboxDomainCreate(dom) 0)
-goto cleanup;
-} else {
-goto cleanup;
+virDomainPtr dom = vboxDomainDefineXML(conn, xml);
+if (dom == NULL)
+return NULL;
+
+if (vboxDomainCreate(dom) 0) {
+vboxDomainUndefine(dom);
+return NULL;
}
return dom;
-
-cleanup:
-vboxDomainUndefine(dom);
-return NULL;
}
static virDomainPtr vboxDomainLookupByID(virConnectPtr conn, int id) {
--
1.6.6.387.g2649b1
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
Matthias Bolte wrote:
...
The original code also contains a domain pointer reference leak.
vboxDomainUndefine does not unref the domain pointer, so there is a
virUnrefDomain call missing in the error case. So this could be
changed to
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
virUnrefDomain(dom);
return NULL;
}
return dom;
But I think this is worth a separate commit.
Indeed.
Here's the proposed patch:
From e47e126afe21b55a3f94a4cde3d9e0b4616de9cb Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 5 Jan 2010 17:45:46 +0100
Subject: [PATCH] vbox_tmpl.c: don't leak a domain pointer upon failure to create
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Unref the domain
upon failure. Patch by Mattias Bolte.
---
src/vbox/vbox_tmpl.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 5889f32..07696c0 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -1004,6 +1004,7 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr
conn, const char *xml,
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
+virUnrefDomain(dom);
return NULL;
}
--
1.6.6.387.g2649b1
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
2010/1/5 Jim Meyering j...@meyering.net:
Matthias Bolte wrote:
2009/12/16 Jim Meyering j...@meyering.net:
Similar to others,
From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 16 Dec 2009 13:56:57 +0100
Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML
failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call
vboxDomainUndefine on a NULL dom.
---
src/vbox/vbox_tmpl.c | 16 +---
1 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index d6b681c..ba70053 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char
*xml,
unsigned int flags
ATTRIBUTE_UNUSED) {
- virDomainPtr dom = NULL;
-
/* VirtualBox currently doesn't have support for running
* virtual machines without actually defining them and thus
* for time being just define new machine and start it.
@@ -1000,17 +998,13 @@ static virDomainPtr
vboxDomainCreateXML(virConnectPtr conn, const char *xml,
* change this behaviour to the expected one.
*/
- dom = vboxDomainDefineXML(conn, xml);
- if (dom) {
- if (vboxDomainCreate(dom) 0)
- goto cleanup;
- } else {
- goto cleanup;
- }
+ virDomainPtr dom = vboxDomainDefineXML(conn, xml);
+ if (dom == NULL)
+ return NULL;
- return dom;
+ if (0 vboxDomainCreate(dom))
+ return dom;
This check is wrong. You meant to check for !(vboxDomainCreate(dom)
0) but resolved the ! incorrectly.
I reversed the condition, but left off the =.
Either change it to
if (vboxDomainCreate(dom) = 0)
return dom;
or keep the negative check
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
return NULL;
}
return dom;
I prefer the second version.
So do I.
Here's the adjusted patch.
From 7e8355ea7c2e50995b139322de574ea4abf24fe3 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 16 Dec 2009 13:56:57 +0100
Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML
failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call
vboxDomainUndefine on a NULL dom.
---
src/vbox/vbox_tmpl.c | 19 +++
1 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 5a1d8dc..5889f32 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml,
unsigned int flags ATTRIBUTE_UNUSED) {
- virDomainPtr dom = NULL;
-
/* VirtualBox currently doesn't have support for running
* virtual machines without actually defining them and thus
* for time being just define new machine and start it.
@@ -1000,19 +998,16 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr
conn, const char *xml,
* change this behaviour to the expected one.
*/
- dom = vboxDomainDefineXML(conn, xml);
- if (dom) {
- if (vboxDomainCreate(dom) 0)
- goto cleanup;
- } else {
- goto cleanup;
+ virDomainPtr dom = vboxDomainDefineXML(conn, xml);
+ if (dom == NULL)
+ return NULL;
+
+ if (vboxDomainCreate(dom) 0) {
+ vboxDomainUndefine(dom);
+ return NULL;
}
return dom;
-
-cleanup:
- vboxDomainUndefine(dom);
- return NULL;
}
static virDomainPtr vboxDomainLookupByID(virConnectPtr conn, int id) {
--
1.6.6.387.g2649b1
ACK.
Matthias
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
2010/1/5 Jim Meyering j...@meyering.net:
Matthias Bolte wrote:
...
The original code also contains a domain pointer reference leak.
vboxDomainUndefine does not unref the domain pointer, so there is a
virUnrefDomain call missing in the error case. So this could be
changed to
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
virUnrefDomain(dom);
return NULL;
}
return dom;
But I think this is worth a separate commit.
Indeed.
Here's the proposed patch:
From e47e126afe21b55a3f94a4cde3d9e0b4616de9cb Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 5 Jan 2010 17:45:46 +0100
Subject: [PATCH] vbox_tmpl.c: don't leak a domain pointer upon failure to
create
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Unref the domain
upon failure. Patch by Mattias Bolte.
I would like my name to be spelled correctly with 'tth' :)
---
src/vbox/vbox_tmpl.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 5889f32..07696c0 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -1004,6 +1004,7 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr
conn, const char *xml,
if (vboxDomainCreate(dom) 0) {
vboxDomainUndefine(dom);
+ virUnrefDomain(dom);
return NULL;
}
--
1.6.6.387.g2649b1
ACK, when my name is spelled correct.
Matthias
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
Matthias Bolte wrote: ... Here's the adjusted patch. Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure * src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call vboxDomainUndefine on a NULL dom. ACK. Pushed. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
Matthias Bolte wrote: ... Subject: [PATCH] vbox_tmpl.c: don't leak a domain pointer upon failure to create * src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Unref the domain upon failure. Patch by Mattias Bolte. I would like my name to be spelled correctly with 'tth' :) ... Fixed and pushed. Thanks. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
