On 15.06.2018 14:53, Igor Mammedov wrote: > On Fri, 15 Jun 2018 13:24:58 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> We don't allow to modify it after realization. So we can simply turn >> it into a static property. The value is now validated during realize(). >> >> Signed-off-by: David Hildenbrand <da...@redhat.com> >> --- >> hw/mem/nvdimm.c | 53 ++++++++----------------------------------------- >> 1 file changed, 8 insertions(+), 45 deletions(-) >> >> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c >> index 7260c9c6b1..d3e8a5bcbb 100644 >> --- a/hw/mem/nvdimm.c >> +++ b/hw/mem/nvdimm.c >> @@ -27,50 +27,6 @@ >> #include "qapi/visitor.h" >> #include "hw/mem/nvdimm.h" >> >> -static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name, >> - void *opaque, Error **errp) >> -{ >> - NVDIMMDevice *nvdimm = NVDIMM(obj); >> - uint64_t value = nvdimm->label_size; >> - >> - visit_type_size(v, name, &value, errp); >> -} >> - >> -static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name, >> - void *opaque, Error **errp) >> -{ >> - NVDIMMDevice *nvdimm = NVDIMM(obj); >> - Error *local_err = NULL; >> - uint64_t value; >> - >> - if (memory_region_size(&nvdimm->nvdimm_mr)) { >> - error_setg(&local_err, "cannot change property value"); >> - goto out; >> - } >> - >> - visit_type_size(v, name, &value, &local_err); >> - if (local_err) { >> - goto out; >> - } >> - if (value < MIN_NAMESPACE_LABEL_SIZE) { >> - error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is >> required" >> - " at least 0x%lx", object_get_typename(obj), >> - name, value, MIN_NAMESPACE_LABEL_SIZE); >> - goto out; >> - } > doesn't seem right, > property setter should throw out error at the time it's set if possible. > > I'd keep this one check where it is and property dynamic. > > and fix only access to uninitialized "if > (memory_region_size(&nvdimm->nvdimm_mr)) {"
Do we really want to simulate a static property with 25+ LOC? But if you insist, to get this stuff of my list, I will turn the if (memory_region_size(&nvdimm->nvdimm_mr)) { into a if (dev->realized) And allow setting the property to 0, too (which is also broken right now). -- Thanks, David / dhildenb