* [Qemu-devel] [PATCH 1/6] blockdev: fix drive_init() opts and bs_opts leaks
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails Stefan Hajnoczi
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
These memory leaks also make drive_add if=none,id=drive0 without a file=
option leak the options list. This keeps ID "drive0" around forever.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
blockdev.c | 51 +++++++++++++++++++++++++++------------------------
1 file changed, 27 insertions(+), 24 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e70e16e..c053a58 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -360,7 +360,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (error_is_set(&error)) {
qerror_report_err(error);
error_free(error);
- return NULL;
+ goto early_err;
}
if (id) {
@@ -390,7 +390,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
;
if (type == IF_COUNT) {
error_report("unsupported bus type '%s'", buf);
- return NULL;
+ goto early_err;
}
} else {
type = block_default_type;
@@ -401,15 +401,15 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (cyls || heads || secs) {
if (cyls < 1) {
error_report("invalid physical cyls number");
- return NULL;
+ goto early_err;
}
if (heads < 1) {
error_report("invalid physical heads number");
- return NULL;
+ goto early_err;
}
if (secs < 1) {
error_report("invalid physical secs number");
- return NULL;
+ goto early_err;
}
}
@@ -417,7 +417,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (!cyls) {
error_report("'%s' trans must be used with cyls, heads and secs",
buf);
- return NULL;
+ goto early_err;
}
if (!strcmp(buf, "none"))
translation = BIOS_ATA_TRANSLATION_NONE;
@@ -427,7 +427,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
translation = BIOS_ATA_TRANSLATION_AUTO;
else {
error_report("'%s' invalid translation type", buf);
- return NULL;
+ goto early_err;
}
}
@@ -437,19 +437,19 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
} else if (!strcmp(buf, "cdrom")) {
if (cyls || secs || heads) {
error_report("CHS can't be set with media=%s", buf);
- return NULL;
+ goto early_err;
}
media = MEDIA_CDROM;
} else {
error_report("'%s' invalid media", buf);
- return NULL;
+ goto early_err;
}
}
if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
error_report("invalid discard option");
- return NULL;
+ goto early_err;
}
}
@@ -471,7 +471,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
/* this is the default */
} else {
error_report("invalid aio option");
- return NULL;
+ goto early_err;
}
}
#endif
@@ -481,7 +481,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
error_printf("Supported formats:");
bdrv_iterate_format(bdrv_format_print, NULL);
error_printf("\n");
- return NULL;
+ goto early_err;
}
drv = bdrv_find_whitelisted_format(buf, ro);
@@ -491,7 +491,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
} else {
error_report("'%s' invalid format", buf);
}
- return NULL;
+ goto early_err;
}
}
@@ -512,7 +512,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (!do_check_io_limits(&io_limits, &error)) {
error_report("%s", error_get_pretty(error));
error_free(error);
- return NULL;
+ goto early_err;
}
if (qemu_opt_get(opts, "boot") != NULL) {
@@ -525,12 +525,12 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
error_report("werror is not supported by this bus type");
- return NULL;
+ goto early_err;
}
on_write_error = parse_block_error_action(buf, 0);
if (on_write_error < 0) {
- return NULL;
+ goto early_err;
}
}
@@ -538,19 +538,19 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
error_report("rerror is not supported by this bus type");
- return NULL;
+ goto early_err;
}
on_read_error = parse_block_error_action(buf, 1);
if (on_read_error < 0) {
- return NULL;
+ goto early_err;
}
}
if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) {
if (type != IF_VIRTIO) {
error_report("addr is not supported by this bus type");
- return NULL;
+ goto early_err;
}
}
@@ -559,7 +559,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (index != -1) {
if (bus_id != 0 || unit_id != -1) {
error_report("index cannot be used with bus and unit");
- return NULL;
+ goto early_err;
}
bus_id = drive_index_to_bus_id(type, index);
unit_id = drive_index_to_unit_id(type, index);
@@ -585,7 +585,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (max_devs && unit_id >= max_devs) {
error_report("unit %d too big (max is %d)",
unit_id, max_devs - 1);
- return NULL;
+ goto early_err;
}
/*
@@ -595,7 +595,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (drive_get(type, bus_id, unit_id) != NULL) {
error_report("drive with bus=%d, unit=%d (index=%d) exists",
bus_id, unit_id, index);
- return NULL;
+ goto early_err;
}
/* init */
@@ -672,6 +672,8 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
if (has_driver_specific_opts) {
file = NULL;
} else {
+ QDECREF(bs_opts);
+ qemu_opts_del(opts);
return dinfo;
}
}
@@ -730,12 +732,13 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
return dinfo;
err:
- qemu_opts_del(opts);
- QDECREF(bs_opts);
bdrv_delete(dinfo->bdrv);
g_free(dinfo->id);
QTAILQ_REMOVE(&drives, dinfo, next);
g_free(dinfo);
+early_err:
+ QDECREF(bs_opts);
+ qemu_opts_del(opts);
return NULL;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 1/6] blockdev: fix drive_init() opts and bs_opts leaks Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
2013-09-10 16:49 ` Paolo Bonzini
2013-09-10 16:21 ` [Qemu-devel] [PATCH 3/6] libqtest: rename qmp() to qmp_discard_response() Stefan Hajnoczi
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
qdev_device_add() leaks the created qdev upon failure. I suspect this
problem crept in because qdev_free() unparents the qdev but does not
drop a reference - confusing name.
Also drop trailing whitespace after curly bracket.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
qdev-monitor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..5657cdc 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
}
if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
qdev_free(qdev);
+ object_unref(OBJECT(qdev));
return NULL;
}
if (qdev->id) {
@@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
object_property_add_child(qdev_get_peripheral_anon(), name,
OBJECT(qdev), NULL);
g_free(name);
- }
+ }
if (qdev_init(qdev) < 0) {
+ object_unref(OBJECT(qdev));
qerror_report(QERR_DEVICE_INIT_FAILED, driver);
return NULL;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails
2013-09-10 16:21 ` [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails Stefan Hajnoczi
@ 2013-09-10 16:49 ` Paolo Bonzini
2013-09-10 16:59 ` Andreas Färber
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2013-09-10 16:49 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Markus Armbruster, Michael Tokarev, qemu-devel, Anthony Liguori,
Andreas Faerber
Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
> qdev_device_add() leaks the created qdev upon failure. I suspect this
> problem crept in because qdev_free() unparents the qdev but does not
> drop a reference - confusing name.
Right, the name a leftover from pre-refcounting days.
BTW, not dropping a reference is the right thing to do because the
reference is dropped much earlier, typically as soon as qdev_device_add
returns. The QOM object tree then will still provide means to access
devices, until they are unparented.
In this case, however, qdev_device_add's caller does not have a
reference to free; doing that is the responsibility of qdev_device_add,
since it returns NULL.
> Also drop trailing whitespace after curly bracket.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> qdev-monitor.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..5657cdc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> }
> if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
> qdev_free(qdev);
> + object_unref(OBJECT(qdev));
> return NULL;
> }
> if (qdev->id) {
> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> object_property_add_child(qdev_get_peripheral_anon(), name,
> OBJECT(qdev), NULL);
> g_free(name);
> - }
> + }
> if (qdev_init(qdev) < 0) {
> + object_unref(OBJECT(qdev));
> qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> return NULL;
> }
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails
2013-09-10 16:49 ` Paolo Bonzini
@ 2013-09-10 16:59 ` Andreas Färber
2013-09-10 17:04 ` Paolo Bonzini
2013-09-11 5:56 ` Stefan Hajnoczi
0 siblings, 2 replies; 11+ messages in thread
From: Andreas Färber @ 2013-09-10 16:59 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Michael Tokarev, qemu-devel, Anthony Liguori,
Markus Armbruster
Am 10.09.2013 18:49, schrieb Paolo Bonzini:
> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>> qdev_device_add() leaks the created qdev upon failure. I suspect this
>> problem crept in because qdev_free() unparents the qdev but does not
>> drop a reference - confusing name.
>
> Right, the name a leftover from pre-refcounting days.
>
> BTW, not dropping a reference is the right thing to do because the
> reference is dropped much earlier, typically as soon as qdev_device_add
> returns. The QOM object tree then will still provide means to access
> devices, until they are unparented.
>
> In this case, however, qdev_device_add's caller does not have a
> reference to free; doing that is the responsibility of qdev_device_add,
> since it returns NULL.
>
>> Also drop trailing whitespace after curly bracket.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> qdev-monitor.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 410cdcb..5657cdc 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>> }
>> if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>> qdev_free(qdev);
>> + object_unref(OBJECT(qdev));
>> return NULL;
>> }
>> if (qdev->id) {
Given that qdev_free() doesn't do what one might expect, I would suggest
to s/qdev_free/object_unparent/g above.
>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>> object_property_add_child(qdev_get_peripheral_anon(), name,
>> OBJECT(qdev), NULL);
>> g_free(name);
>> - }
>> + }
>> if (qdev_init(qdev) < 0) {
>> + object_unref(OBJECT(qdev));
>> qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>> return NULL;
>> }
>>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
I would like to take this through qom-next tree since I have pending
variable cleanups there ("qdev" being touched here). Not sure how to
handle that wrt block changes in this series?
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails
2013-09-10 16:59 ` Andreas Färber
@ 2013-09-10 17:04 ` Paolo Bonzini
2013-09-11 5:56 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2013-09-10 17:04 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Michael Tokarev, qemu-devel, Stefan Hajnoczi,
Markus Armbruster
Il 10/09/2013 18:59, Andreas Färber ha scritto:
> Am 10.09.2013 18:49, schrieb Paolo Bonzini:
>> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>>> qdev_device_add() leaks the created qdev upon failure. I suspect this
>>> problem crept in because qdev_free() unparents the qdev but does not
>>> drop a reference - confusing name.
>>
>> Right, the name a leftover from pre-refcounting days.
>>
>> BTW, not dropping a reference is the right thing to do because the
>> reference is dropped much earlier, typically as soon as qdev_device_add
>> returns. The QOM object tree then will still provide means to access
>> devices, until they are unparented.
>>
>> In this case, however, qdev_device_add's caller does not have a
>> reference to free; doing that is the responsibility of qdev_device_add,
>> since it returns NULL.
>>
>>> Also drop trailing whitespace after curly bracket.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> qdev-monitor.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 410cdcb..5657cdc 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>> }
>>> if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>> qdev_free(qdev);
>>> + object_unref(OBJECT(qdev));
>>> return NULL;
>>> }
>>> if (qdev->id) {
>
> Given that qdev_free() doesn't do what one might expect, I would suggest
> to s/qdev_free/object_unparent/g above.
Then do it everywhere...
Paolo
>>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>> object_property_add_child(qdev_get_peripheral_anon(), name,
>>> OBJECT(qdev), NULL);
>>> g_free(name);
>>> - }
>>> + }
>>> if (qdev_init(qdev) < 0) {
>>> + object_unref(OBJECT(qdev));
>>> qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>> return NULL;
>>> }
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> I would like to take this through qom-next tree since I have pending
> variable cleanups there ("qdev" being touched here). Not sure how to
> handle that wrt block changes in this series?
>
> Andreas
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails
2013-09-10 16:59 ` Andreas Färber
2013-09-10 17:04 ` Paolo Bonzini
@ 2013-09-11 5:56 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-11 5:56 UTC (permalink / raw)
To: Andreas Färber
Cc: Stefan Hajnoczi, Michael Tokarev, qemu-devel, Markus Armbruster,
Anthony Liguori, Paolo Bonzini
On Tue, Sep 10, 2013 at 6:59 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.09.2013 18:49, schrieb Paolo Bonzini:
>> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>>> qdev_device_add() leaks the created qdev upon failure. I suspect this
>>> problem crept in because qdev_free() unparents the qdev but does not
>>> drop a reference - confusing name.
>>
>> Right, the name a leftover from pre-refcounting days.
>>
>> BTW, not dropping a reference is the right thing to do because the
>> reference is dropped much earlier, typically as soon as qdev_device_add
>> returns. The QOM object tree then will still provide means to access
>> devices, until they are unparented.
>>
>> In this case, however, qdev_device_add's caller does not have a
>> reference to free; doing that is the responsibility of qdev_device_add,
>> since it returns NULL.
>>
>>> Also drop trailing whitespace after curly bracket.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> qdev-monitor.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 410cdcb..5657cdc 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>> }
>>> if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>> qdev_free(qdev);
>>> + object_unref(OBJECT(qdev));
>>> return NULL;
>>> }
>>> if (qdev->id) {
>
> Given that qdev_free() doesn't do what one might expect, I would suggest
> to s/qdev_free/object_unparent/g above.
I'll send a follow-up patch to do this across the tree.
>>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>> object_property_add_child(qdev_get_peripheral_anon(), name,
>>> OBJECT(qdev), NULL);
>>> g_free(name);
>>> - }
>>> + }
>>> if (qdev_init(qdev) < 0) {
>>> + object_unref(OBJECT(qdev));
>>> qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>> return NULL;
>>> }
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> I would like to take this through qom-next tree since I have pending
> variable cleanups there ("qdev" being touched here). Not sure how to
> handle that wrt block changes in this series?
It would be simplest to take the whole series. Splitting it is hard
because the qdev device_add test case requires the blockdev.c fix
(otherwise the drive ID is held due to the leaked options list).
Or you can cherry-pick just this patch and the rest of the series can
go via the block tree?
Stefan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/6] libqtest: rename qmp() to qmp_discard_response()
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 1/6] blockdev: fix drive_init() opts and bs_opts leaks Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 4/6] libqtest: add qmp(fmt, ...) -> QDict* function Stefan Hajnoczi
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
Existing qmp() callers do not expect a response object. In order to
implement real QMP test cases it will be necessary to inspect the
response object.
Rename qmp() to qmp_discard_response(). Later patches will introduce a
qmp() function that returns the response object and tests that use it.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/boot-order-test.c | 4 ++--
tests/fdc-test.c | 15 +++++++++------
tests/ide-test.c | 10 ++++++----
tests/libqtest.c | 8 ++++----
tests/libqtest.h | 20 ++++++++++----------
5 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 4b233d0..da158c3 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -41,12 +41,12 @@ static void test_a_boot_order(const char *machine,
qtest_start(args);
actual = read_boot_order();
g_assert_cmphex(actual, ==, expected_boot);
- qmp("{ 'execute': 'system_reset' }");
+ qmp_discard_response("{ 'execute': 'system_reset' }");
/*
* system_reset only requests reset. We get a RESET event after
* the actual reset completes. Need to wait for that.
*/
- qmp(""); /* HACK: wait for event */
+ qmp_discard_response(""); /* HACK: wait for event */
actual = read_boot_order();
g_assert_cmphex(actual, ==, expected_reboot);
qtest_quit(global_qtest);
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index fd198dc..38b5b17 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -290,10 +290,12 @@ static void test_media_insert(void)
/* Insert media in drive. DSKCHK should not be reset until a step pulse
* is sent. */
- qmp("{'execute':'change', 'arguments':{ 'device':'floppy0', "
- "'target': '%s' }}", test_image);
- qmp(""); /* ignore event (FIXME open -> open transition?!) */
- qmp(""); /* ignore event */
+ qmp_discard_response("{'execute':'change', 'arguments':{"
+ " 'device':'floppy0', 'target': '%s' }}",
+ test_image);
+ qmp_discard_response(""); /* ignore event
+ (FIXME open -> open transition?!) */
+ qmp_discard_response(""); /* ignore event */
dir = inb(FLOPPY_BASE + reg_dir);
assert_bit_set(dir, DSKCHG);
@@ -322,8 +324,9 @@ static void test_media_change(void)
/* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
* reset the bit. */
- qmp("{'execute':'eject', 'arguments':{ 'device':'floppy0' }}");
- qmp(""); /* ignore event */
+ qmp_discard_response("{'execute':'eject', 'arguments':{"
+ " 'device':'floppy0' }}");
+ qmp_discard_response(""); /* ignore event */
dir = inb(FLOPPY_BASE + reg_dir);
assert_bit_set(dir, DSKCHG);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 7307f1d..0e6fa5d 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -435,8 +435,9 @@ static void test_flush(void)
tmp_path);
/* Delay the completion of the flush request until we explicitly do it */
- qmp("{'execute':'human-monitor-command', 'arguments': { "
- "'command-line': 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
+ qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
+ " 'command-line':"
+ " 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
/* FLUSH CACHE command on device 0*/
outb(IDE_BASE + reg_device, 0);
@@ -448,8 +449,9 @@ static void test_flush(void)
assert_bit_clear(data, DF | ERR | DRQ);
/* Complete the command */
- qmp("{'execute':'human-monitor-command', 'arguments': { "
- "'command-line': 'qemu-io ide0-hd0 \"resume A\"'} }");
+ qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
+ " 'command-line':"
+ " 'qemu-io ide0-hd0 \"resume A\"'} }");
/* Check registers */
data = inb(IDE_BASE + reg_device);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index bb82069..d46ad02 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -151,8 +151,8 @@ QTestState *qtest_init(const char *extra_args)
}
/* Read the QMP greeting and then do the handshake */
- qtest_qmp(s, "");
- qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }");
+ qtest_qmp_discard_response(s, "");
+ qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
if (getenv("QTEST_STOP")) {
kill(qtest_qemu_pid(s), SIGSTOP);
@@ -291,7 +291,7 @@ redo:
return words;
}
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
{
bool has_reply = false;
int nesting = 0;
@@ -326,7 +326,7 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
}
}
-void qtest_qmp(QTestState *s, const char *fmt, ...)
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
{
va_list ap;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index a6e99bd..4f1b060 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -44,23 +44,23 @@ QTestState *qtest_init(const char *extra_args);
void qtest_quit(QTestState *s);
/**
- * qtest_qmp:
+ * qtest_qmp_discard_response:
* @s: #QTestState instance to operate on.
* @fmt...: QMP message to send to qemu
*
- * Sends a QMP message to QEMU
+ * Sends a QMP message to QEMU and consumes the response.
*/
-void qtest_qmp(QTestState *s, const char *fmt, ...);
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
/**
- * qtest_qmpv:
+ * qtest_qmpv_discard_response:
* @s: #QTestState instance to operate on.
* @fmt: QMP message to send to QEMU
* @ap: QMP message arguments
*
- * Sends a QMP message to QEMU.
+ * Sends a QMP message to QEMU and consumes the response.
*/
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
/**
* qtest_get_irq:
@@ -331,17 +331,17 @@ static inline void qtest_end(void)
}
/**
- * qmp:
+ * qmp_discard_response:
* @fmt...: QMP message to send to qemu
*
- * Sends a QMP message to QEMU
+ * Sends a QMP message to QEMU and consumes the response.
*/
-static inline void qmp(const char *fmt, ...)
+static inline void qmp_discard_response(const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
- qtest_qmpv(global_qtest, fmt, ap);
+ qtest_qmpv_discard_response(global_qtest, fmt, ap);
va_end(ap);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 4/6] libqtest: add qmp(fmt, ...) -> QDict* function
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
` (2 preceding siblings ...)
2013-09-10 16:21 ` [Qemu-devel] [PATCH 3/6] libqtest: rename qmp() to qmp_discard_response() Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 5/6] blockdev-test: add test case for drive_add duplicate IDs Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 6/6] qdev-monitor-test: add device_add leak test cases Stefan Hajnoczi
5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
Add a qtest qmp() function that returns the response object. This
allows test cases to verify the result or to check for error responses.
It also allows waiting for QMP events.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/libqtest.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------------
tests/libqtest.h | 37 +++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+), 14 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index d46ad02..83424c3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -30,6 +30,8 @@
#include "qemu/compiler.h"
#include "qemu/osdep.h"
+#include "qapi/qmp/json-streamer.h"
+#include "qapi/qmp/json-parser.h"
#define MAX_IRQ 256
@@ -291,16 +293,38 @@ redo:
return words;
}
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+typedef struct {
+ JSONMessageParser parser;
+ QDict *response;
+} QMPResponseParser;
+
+static void qmp_response(JSONMessageParser *parser, QList *tokens)
{
- bool has_reply = false;
- int nesting = 0;
+ QMPResponseParser *qmp = container_of(parser, QMPResponseParser, parser);
+ QObject *obj;
+
+ obj = json_parser_parse(tokens, NULL);
+ if (!obj) {
+ fprintf(stderr, "QMP JSON response parsing failed\n");
+ exit(1);
+ }
+
+ g_assert(qobject_type(obj) == QTYPE_QDICT);
+ g_assert(!qmp->response);
+ qmp->response = (QDict *)obj;
+}
+
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+{
+ QMPResponseParser qmp;
/* Send QMP request */
socket_sendf(s->qmp_fd, fmt, ap);
/* Receive reply */
- while (!has_reply || nesting > 0) {
+ qmp.response = NULL;
+ json_message_parser_init(&qmp.parser, qmp_response);
+ while (!qmp.response) {
ssize_t len;
char c;
@@ -314,25 +338,39 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
exit(1);
}
- switch (c) {
- case '{':
- nesting++;
- has_reply = true;
- break;
- case '}':
- nesting--;
- break;
- }
+ json_message_parser_feed(&qmp.parser, &c, 1);
}
+ json_message_parser_destroy(&qmp.parser);
+
+ return qmp.response;
+}
+
+QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
+{
+ va_list ap;
+ QDict *response;
+
+ va_start(ap, fmt);
+ response = qtest_qmpv(s, fmt, ap);
+ va_end(ap);
+ return response;
+}
+
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+{
+ QDict *response = qtest_qmpv(s, fmt, ap);
+ QDECREF(response);
}
void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
{
va_list ap;
+ QDict *response;
va_start(ap, fmt);
- qtest_qmpv(s, fmt, ap);
+ response = qtest_qmpv(s, fmt, ap);
va_end(ap);
+ QDECREF(response);
}
const char *qtest_get_arch(void)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 4f1b060..9deebdc 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -22,6 +22,7 @@
#include <stdbool.h>
#include <stdarg.h>
#include <sys/types.h>
+#include "qapi/qmp/qdict.h"
typedef struct QTestState QTestState;
@@ -53,6 +54,15 @@ void qtest_quit(QTestState *s);
void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
/**
+ * qtest_qmp:
+ * @s: #QTestState instance to operate on.
+ * @fmt...: QMP message to send to qemu
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
+
+/**
* qtest_qmpv_discard_response:
* @s: #QTestState instance to operate on.
* @fmt: QMP message to send to QEMU
@@ -63,6 +73,16 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
/**
+ * qtest_qmpv:
+ * @s: #QTestState instance to operate on.
+ * @fmt: QMP message to send to QEMU
+ * @ap: QMP message arguments
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+
+/**
* qtest_get_irq:
* @s: #QTestState instance to operate on.
* @num: Interrupt to observe.
@@ -331,6 +351,23 @@ static inline void qtest_end(void)
}
/**
+ * qmp:
+ * @fmt...: QMP message to send to qemu
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+static inline QDict *qmp(const char *fmt, ...)
+{
+ va_list ap;
+ QDict *response;
+
+ va_start(ap, fmt);
+ response = qtest_qmpv(global_qtest, fmt, ap);
+ va_end(ap);
+ return response;
+}
+
+/**
* qmp_discard_response:
* @fmt...: QMP message to send to qemu
*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 5/6] blockdev-test: add test case for drive_add duplicate IDs
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
` (3 preceding siblings ...)
2013-09-10 16:21 ` [Qemu-devel] [PATCH 4/6] libqtest: add qmp(fmt, ...) -> QDict* function Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
2013-09-10 16:21 ` [Qemu-devel] [PATCH 6/6] qdev-monitor-test: add device_add leak test cases Stefan Hajnoczi
5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
The following should work:
(qemu) drive_add if=none,id=drive0
(qemu) drive_del drive0
(qemu) drive_add if=none,id=drive0
Previous versions of QEMU produced a duplicate ID error because
drive_add leaked the options.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/Makefile | 2 ++
tests/blockdev-test.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
create mode 100644 tests/blockdev-test.c
diff --git a/tests/Makefile b/tests/Makefile
index baba9e9..fe71ec9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -66,6 +66,7 @@ check-qtest-i386-y += tests/boot-order-test$(EXESUF)
check-qtest-i386-y += tests/rtc-test$(EXESUF)
check-qtest-i386-y += tests/i440fx-test$(EXESUF)
check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
+check-qtest-i386-y += tests/blockdev-test$(EXESUF)
check-qtest-x86_64-y = $(check-qtest-i386-y)
gcov-files-i386-y += i386-softmmu/hw/mc146818rtc.c
gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -172,6 +173,7 @@ tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
# QTest rules
diff --git a/tests/blockdev-test.c b/tests/blockdev-test.c
new file mode 100644
index 0000000..8b7371e
--- /dev/null
+++ b/tests/blockdev-test.c
@@ -0,0 +1,59 @@
+/*
+ * blockdev.c test cases
+ *
+ * Copyright (C) 2013 Red Hat Inc.
+ *
+ * Authors:
+ * Stefan Hajnoczi <stefanha@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <string.h>
+#include "libqtest.h"
+
+static void test_drive_add_empty(void)
+{
+ QDict *response;
+ const char *response_return;
+
+ /* Start with an empty drive */
+ qtest_start("-drive if=none,id=drive0");
+
+ /* Delete the drive */
+ response = qmp("{\"execute\": \"human-monitor-command\","
+ " \"arguments\": {"
+ " \"command-line\": \"drive_del drive0\""
+ "}}");
+ g_assert(response);
+ response_return = qdict_get_try_str(response, "return");
+ g_assert(response_return);
+ g_assert(strcmp(response_return, "") == 0);
+ QDECREF(response);
+
+ /* Ensure re-adding the drive works - there should be no duplicate ID error
+ * because the old drive must be gone.
+ */
+ response = qmp("{\"execute\": \"human-monitor-command\","
+ " \"arguments\": {"
+ " \"command-line\": \"drive_add 0 if=none,id=drive0\""
+ "}}");
+ g_assert(response);
+ response_return = qdict_get_try_str(response, "return");
+ g_assert(response_return);
+ g_assert(strcmp(response_return, "OK\r\n") == 0);
+ QDECREF(response);
+
+ qtest_end();
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("/qdev/drive_add_empty", test_drive_add_empty);
+
+ return g_test_run();
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 6/6] qdev-monitor-test: add device_add leak test cases
2013-09-10 16:21 [Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes Stefan Hajnoczi
` (4 preceding siblings ...)
2013-09-10 16:21 ` [Qemu-devel] [PATCH 5/6] blockdev-test: add test case for drive_add duplicate IDs Stefan Hajnoczi
@ 2013-09-10 16:21 ` Stefan Hajnoczi
5 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2013-09-10 16:21 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael Tokarev, Markus Armbruster,
Stefan Hajnoczi, Paolo Bonzini, Andreas Faerber
Ensure that the device_add error code path deletes device objects.
Failure to do so not only leaks the objects but can also keep other
objects (like drive or netdev) alive due to qdev properties holding
references.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/Makefile | 2 ++
tests/qdev-monitor-test.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+)
create mode 100644 tests/qdev-monitor-test.c
diff --git a/tests/Makefile b/tests/Makefile
index fe71ec9..dc3b8f1 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -67,6 +67,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
check-qtest-i386-y += tests/i440fx-test$(EXESUF)
check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
check-qtest-i386-y += tests/blockdev-test$(EXESUF)
+check-qtest-i386-y += tests/qdev-monitor-test$(EXESUF)
check-qtest-x86_64-y = $(check-qtest-i386-y)
gcov-files-i386-y += i386-softmmu/hw/mc146818rtc.c
gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -174,6 +175,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
+tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
# QTest rules
diff --git a/tests/qdev-monitor-test.c b/tests/qdev-monitor-test.c
new file mode 100644
index 0000000..d3d6d39
--- /dev/null
+++ b/tests/qdev-monitor-test.c
@@ -0,0 +1,81 @@
+/*
+ * qdev-monitor.c test cases
+ *
+ * Copyright (C) 2013 Red Hat Inc.
+ *
+ * Authors:
+ * Stefan Hajnoczi <stefanha@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include <string.h>
+#include <glib.h>
+#include "libqtest.h"
+#include "qapi/qmp/qjson.h"
+
+static void test_device_add(void)
+{
+ QDict *response;
+ QDict *error;
+
+ qtest_start("-drive if=none,id=drive0");
+
+ /* Make device_add fail. If this leaks the virtio-blk-pci device then a
+ * reference to drive0 will also be held (via qdev properties).
+ */
+ response = qmp("{\"execute\": \"device_add\","
+ " \"arguments\": {"
+ " \"driver\": \"virtio-blk-pci\","
+ " \"drive\": \"drive0\""
+ "}}");
+ g_assert(response);
+ error = qdict_get_qdict(response, "error");
+ g_assert(!strcmp(qdict_get_try_str(error, "class") ?: "",
+ "GenericError"));
+ g_assert(!strcmp(qdict_get_try_str(error, "desc") ?: "",
+ "Device initialization failed."));
+ QDECREF(response);
+
+ /* Delete the drive */
+ response = qmp("{\"execute\": \"human-monitor-command\","
+ " \"arguments\": {"
+ " \"command-line\": \"drive_del drive0\""
+ "}}");
+ g_assert(response);
+ g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "(null)", ""));
+ QDECREF(response);
+
+ /* Try to re-add the drive. This fails with duplicate IDs if a leaked
+ * virtio-blk-pci exists that holds a reference to the old drive0.
+ */
+ response = qmp("{\"execute\": \"human-monitor-command\","
+ " \"arguments\": {"
+ " \"command-line\": \"drive_add pci-addr=auto if=none,id=drive0\""
+ "}}");
+ g_assert(response);
+ g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "",
+ "OK\r\n"));
+ QDECREF(response);
+
+ qtest_end();
+}
+
+int main(int argc, char **argv)
+{
+ const char *arch = qtest_get_arch();
+
+ /* Check architecture */
+ if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
+ g_test_message("Skipping test for non-x86\n");
+ return 0;
+ }
+
+ /* Run the tests */
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("/qdev/device_add", test_device_add);
+
+ return g_test_run();
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread