qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a st


From: David Hildenbrand
Subject: Re: [Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a static property
Date: Fri, 15 Jun 2018 15:30:09 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

On 15.06.2018 14:53, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 13:24:58 +0200
> David Hildenbrand <address@hidden> 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 <address@hidden>
>> ---
>>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]