Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[David Hildenbrand]

On 16.06.2018 04:05, Haozhong Zhang wrote:
> On 06/15/18 16:04, David Hildenbrand wrote:
>> It is inititally 0, so setting it to 0 should be allowed, too.
> 
> I'm fine with this change and believe nothing is broken in practice,
> but what is expected by the user who sets a zero label size?

I'd say exactly the same as if not specifying a label size, because the
default is initially 0.

I remember that the user should be able to spell everything out on the
cmdline. Relying only on default values is usually not what we want.

> 
> Look at nvdimm_dsm_device() which enables label DSMs only if the label
> size is not smaller than 128 KB. If a user sets a zero label size
> explicitly, does he/she expect those label DSMs are available in
> guest?  (according to Intel spec, the minimal label size is 128
> KBytes)
> 
> I think if it's allowed to set a zero label-size, it would be better
> to document its difference from other non-zero values in docs/nvdimm.txt.

We can fixup the the documentation to to explicitly state "default is
label-size=0" and "With label-size=0 label support is disabled.".

But this will be a separate patch.

Thanks!

> 
> Thanks,
> Haozhong
> 
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/mem/nvdimm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>> index db7d8c3050..df7646488b 100644
>> --- a/hw/mem/nvdimm.c
>> +++ b/hw/mem/nvdimm.c
>> @@ -52,9 +52,9 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, 
>> const char *name,
>>      if (local_err) {
>>          goto out;
>>      }
>> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
>> +    if (value && value < MIN_NAMESPACE_LABEL_SIZE) {
>>          error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is 
>> required"
>> -                   " at least 0x%lx", object_get_typename(obj),
>> +                   " either 0 or at least 0x%lx", object_get_typename(obj),
>>                     name, value, MIN_NAMESPACE_LABEL_SIZE);
>>          goto out;
>>      }
>> -- 
>> 2.17.1
>>
>>
> 


-- 

Thanks,

David / dhildenb