Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0
On 18.06.2018 14:03, Igor Mammedov wrote: > On Fri, 15 Jun 2018 16:04:46 +0200 > David Hildenbrand wrote: > >> It is inititally 0, so setting it to 0 should be allowed, too. > I'm not sure if we need to permit it. > By default labels are disabled (label-size=0) and user are supposed to provide > this option if labels should be enabled with a valid size. > > it could be confusing for user when asking for label and not getting it. > > I suggest to drop this patch, it's not really related to this series > nor required for your future work. > As I just want to finally get this stuff off my table, I agree to whatever you say. -- Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0
On Fri, 15 Jun 2018 16:04:46 +0200
David Hildenbrand wrote:
> It is inititally 0, so setting it to 0 should be allowed, too.
I'm not sure if we need to permit it.
By default labels are disabled (label-size=0) and user are supposed to provide
this option if labels should be enabled with a valid size.
it could be confusing for user when asking for label and not getting it.
I suggest to drop this patch, it's not really related to this series
nor required for your future work.
> Signed-off-by: David Hildenbrand
> ---
> 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(_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;
> }
Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0
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
>> ---
>> 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(_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
Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0
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?
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.
Thanks,
Haozhong
>
> Signed-off-by: David Hildenbrand
> ---
> 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(_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
>
>
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0
It is inititally 0, so setting it to 0 should be allowed, too.
Signed-off-by: David Hildenbrand
---
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(_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
