* [Qemu-devel] [PATCH v2 1/2] qom: Do not reuse errp after a possible error
2015-08-25 18:00 [Qemu-devel] [PATCH v2 0/2] qom: Fix misuse of Error API Markus Armbruster
@ 2015-08-25 18:00 ` Markus Armbruster
2015-08-25 18:00 ` [Qemu-devel] [PATCH v2 2/2] qom: Fix invalid error check in property_get_str() Markus Armbruster
2015-09-03 10:06 ` [Qemu-devel] [PATCH v2 0/2] qom: Fix misuse of Error API Markus Armbruster
2 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2015-08-25 18:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Hu Tao, afaerber, Anthony Liguori
The argument for an Error **errp parameter must point to a null
pointer. If it doesn't, and an error happens, error_set() fails its
assertion.
Instead of
foo(foos, errp);
bar(bars, errp);
you need to do something like
Error *err = NULL;
foo(foos, &err);
if (err) {
error_propagate(errp, err);
goto out;
}
bar(bars, errp);
out:
Screwed up in commit 0e55884 (v1.3.0): property_get_bool().
Screwed up in commit 1f21772 (v2.1.0): object_property_get_enum() and
object_property_get_uint16List().
Screwed up in commit a8e3fbe (v2.4.0): property_get_enum(),
property_set_enum().
Found by inspection, no actual crashes observed.
Fix them up.
Cc: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
qom/object.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index eea8edf..6173da8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1079,6 +1079,7 @@ typedef struct EnumProperty {
int object_property_get_enum(Object *obj, const char *name,
const char *typename, Error **errp)
{
+ Error *err = NULL;
StringOutputVisitor *sov;
StringInputVisitor *siv;
char *str;
@@ -1100,7 +1101,12 @@ int object_property_get_enum(Object *obj, const char *name,
enumprop = prop->opaque;
sov = string_output_visitor_new(false);
- object_property_get(obj, string_output_get_visitor(sov), name, errp);
+ object_property_get(obj, string_output_get_visitor(sov), name, &err);
+ if (err) {
+ error_propagate(errp, err);
+ string_output_visitor_cleanup(sov);
+ return 0;
+ }
str = string_output_get_string(sov);
siv = string_input_visitor_new(str);
string_output_visitor_cleanup(sov);
@@ -1116,21 +1122,27 @@ int object_property_get_enum(Object *obj, const char *name,
void object_property_get_uint16List(Object *obj, const char *name,
uint16List **list, Error **errp)
{
+ Error *err = NULL;
StringOutputVisitor *ov;
StringInputVisitor *iv;
char *str;
ov = string_output_visitor_new(false);
object_property_get(obj, string_output_get_visitor(ov),
- name, errp);
+ name, &err);
+ if (err) {
+ error_propagate(errp, err);
+ goto out;
+ }
str = string_output_get_string(ov);
iv = string_input_visitor_new(str);
visit_type_uint16List(string_input_get_visitor(iv),
list, NULL, errp);
g_free(str);
- string_output_visitor_cleanup(ov);
string_input_visitor_cleanup(iv);
+out:
+ string_output_visitor_cleanup(ov);
}
void object_property_parse(Object *obj, const char *string,
@@ -1644,10 +1656,16 @@ typedef struct BoolProperty
static void property_get_bool(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
+ Error *err = NULL;
BoolProperty *prop = opaque;
bool value;
- value = prop->get(obj, errp);
+ value = prop->get(obj, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+
visit_type_bool(v, &value, name, errp);
}
@@ -1699,20 +1717,31 @@ void object_property_add_bool(Object *obj, const char *name,
static void property_get_enum(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
+ Error *err = NULL;
EnumProperty *prop = opaque;
int value;
- value = prop->get(obj, errp);
+ value = prop->get(obj, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+
visit_type_enum(v, &value, prop->strings, NULL, name, errp);
}
static void property_set_enum(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
+ Error *err = NULL;
EnumProperty *prop = opaque;
int value;
- visit_type_enum(v, &value, prop->strings, NULL, name, errp);
+ visit_type_enum(v, &value, prop->strings, NULL, name, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
prop->set(obj, value, errp);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v2 2/2] qom: Fix invalid error check in property_get_str()
2015-08-25 18:00 [Qemu-devel] [PATCH v2 0/2] qom: Fix misuse of Error API Markus Armbruster
2015-08-25 18:00 ` [Qemu-devel] [PATCH v2 1/2] qom: Do not reuse errp after a possible error Markus Armbruster
@ 2015-08-25 18:00 ` Markus Armbruster
2015-09-03 10:06 ` [Qemu-devel] [PATCH v2 0/2] qom: Fix misuse of Error API Markus Armbruster
2 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2015-08-25 18:00 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber, Anthony Liguori
When a function returns a null pointer on error and only on error, you
can do
if (!foo(foos, errp)) {
... handle error ...
}
instead of the more cumbersome
Error *err = NULL;
if (!foo(foos, &err)) {
error_propagate(errp, err);
... handle error ...
}
A StringProperty's getter, however, may return null on success! We
then fail to call visit_type_str().
Screwed up in 6a146eb, v1.1.
Fails tests/qom-test in my current, heavily hacked QAPI branch. No
reproducer for master known (but I didn't look hard).
Cc: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qom/object.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 6173da8..4c4df55 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1591,14 +1591,18 @@ typedef struct StringProperty
static void property_get_str(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
+ Error *err = NULL;
StringProperty *prop = opaque;
char *value;
- value = prop->get(obj, errp);
- if (value) {
- visit_type_str(v, &value, name, errp);
- g_free(value);
+ value = prop->get(obj, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
}
+
+ visit_type_str(v, &value, name, errp);
+ g_free(value);
}
static void property_set_str(Object *obj, Visitor *v, void *opaque,
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread