| Message ID | 20191130194240.10517-16-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
On 30.11.19 20:42, Markus Armbruster wrote: > s390x-cpu property setters set_feature() and set_feature_group() crash > when the visitor fails and its @errp argument is null. Messed up in > commit 0754f60429 "s390x/cpumodel: expose features and feature groups > as properties". Same comment as to the other patches :) I think we usually use "s390x/cpumodels", but that's just nitpicking. Reviewed-by: David Hildenbrand <david@redhat.com> > > The bug can't bite as no caller actually passes null. Fix it anyway. > > Cc: David Hildenbrand <david@redhat.com> > Cc: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > target/s390x/cpu_models.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 7e92fb2e15..6a29fd3ab1 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char *name, > static void set_feature(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > + Error *err = NULL; > S390Feat feat = (S390Feat) opaque; > DeviceState *dev = DEVICE(obj); > S390CPU *cpu = S390_CPU(obj); > @@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const char *name, > return; > } > > - visit_type_bool(v, name, &value, errp); > - if (*errp) { > + visit_type_bool(v, name, &value, &err); > + if (err) { > + error_propagate(errp, err); > return; > } > if (value) { > @@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, const char *name, > static void set_feature_group(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > + Error *err = NULL; > S390FeatGroup group = (S390FeatGroup) opaque; > const S390FeatGroupDef *def = s390_feat_group_def(group); > DeviceState *dev = DEVICE(obj); > @@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, const char *name, > return; > } > > - visit_type_bool(v, name, &value, errp); > - if (*errp) { > + visit_type_bool(v, name, &value, &err); > + if (err) { > + error_propagate(errp, err); > return; > } > if (value) { >
David Hildenbrand <david@redhat.com> writes: > On 30.11.19 20:42, Markus Armbruster wrote: >> s390x-cpu property setters set_feature() and set_feature_group() crash >> when the visitor fails and its @errp argument is null. Messed up in >> commit 0754f60429 "s390x/cpumodel: expose features and feature groups >> as properties". > > Same comment as to the other patches :) > > I think we usually use "s390x/cpumodels", but that's just nitpicking. $ git-log --oneline target/s390x/cpu_models.c | awk '$2 ~ /:$/ { print $2 }' | sort | uniq -c 1 S390: 6 qapi: 1 qemu-common: 1 qmp: 2 qobject: 1 qom: 1 s390/cpumodel: 1 s390x/ccw: 21 s390x/cpumodel: 1 s390x/cpumodels: 1 s390x/kvm: 4 s390x/tcg: 7 s390x: 1 target/s390x/cpu_models: 17 target/s390x: 1 target: You're right, except for the plural vs. singular. I should've browsed git-log. > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks!
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 7e92fb2e15..6a29fd3ab1 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char *name, static void set_feature(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { + Error *err = NULL; S390Feat feat = (S390Feat) opaque; DeviceState *dev = DEVICE(obj); S390CPU *cpu = S390_CPU(obj); @@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const char *name, return; } - visit_type_bool(v, name, &value, errp); - if (*errp) { + visit_type_bool(v, name, &value, &err); + if (err) { + error_propagate(errp, err); return; } if (value) { @@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, const char *name, static void set_feature_group(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { + Error *err = NULL; S390FeatGroup group = (S390FeatGroup) opaque; const S390FeatGroupDef *def = s390_feat_group_def(group); DeviceState *dev = DEVICE(obj); @@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, const char *name, return; } - visit_type_bool(v, name, &value, errp); - if (*errp) { + visit_type_bool(v, name, &value, &err); + if (err) { + error_propagate(errp, err); return; } if (value) {
s390x-cpu property setters set_feature() and set_feature_group() crash when the visitor fails and its @errp argument is null. Messed up in commit 0754f60429 "s390x/cpumodel: expose features and feature groups as properties". The bug can't bite as no caller actually passes null. Fix it anyway. Cc: David Hildenbrand <david@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- target/s390x/cpu_models.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)