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 <da...@redhat.com> >> --- >> 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