Move 'labelsize' validation to virDomainMemoryDefPostParse().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1534dcc1e..5e5905f483 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5363,15 +5363,28 @@ static int
 virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
                             const virDomainDef *def)
 {
-    /* Although only the QEMU driver implements PPC64 support, this
-     * code is related to the platform specification (PAPR), i.e. it
-     * is hypervisor agnostic, and any future PPC64 hypervisor driver
-     * will have the same restriction.
-     */
-    if (ARCH_IS_PPC64(def->os.arch) &&
-        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-        virDomainNVDimmAlignSizePseries(mem) < 0)
-        return -1;
+    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+        if (mem->labelsize && mem->labelsize < 128) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("nvdimm label must be at least 128KiB"));
+            return -1;
+        }
+
+        if (mem->labelsize >= mem->size) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("label size must be smaller than NVDIMM size"));
+            return -1;
+        }
+
+        /* Although only the QEMU driver implements PPC64 support, this
+         * code is related to the platform specification (PAPR), i.e. it
+         * is hypervisor agnostic, and any future PPC64 hypervisor driver
+         * will have the same restriction.
+         */
+        if (ARCH_IS_PPC64(def->os.arch) &&
+            virDomainNVDimmAlignSizePseries(mem) < 0)
+            return -1;
+    }
 
     return 0;
 }
@@ -16766,18 +16779,6 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
                                  &def->labelsize, false, false) < 0)
             return -1;
 
-        if (def->labelsize && def->labelsize < 128) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("nvdimm label must be at least 128KiB"));
-            return -1;
-        }
-
-        if (def->labelsize >= def->size) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("label size must be smaller than NVDIMM size"));
-            return -1;
-        }
-
         if (virXPathBoolean("boolean(./readonly)", ctxt))
             def->readonly = true;
     }
-- 
2.26.2

On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
> Move 'labelsize' validation to virDomainMemoryDefPostParse().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
>   1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b1534dcc1e..5e5905f483 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5363,15 +5363,28 @@ static int
>   virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
>                               const virDomainDef *def)
>   {
> -    /* Although only the QEMU driver implements PPC64 support, this
> -     * code is related to the platform specification (PAPR), i.e. it
> -     * is hypervisor agnostic, and any future PPC64 hypervisor driver
> -     * will have the same restriction.
> -     */
> -    if (ARCH_IS_PPC64(def->os.arch) &&
> -        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
> -        virDomainNVDimmAlignSizePseries(mem) < 0)
> -        return -1;
> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> +        if (mem->labelsize && mem->labelsize < 128) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("nvdimm label must be at least 128KiB"));
> +            return -1;
> +        }
> +
> +        if (mem->labelsize >= mem->size) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("label size must be smaller than NVDIMM size"));
> +            return -1;
> +        }
> +
> +        /* Although only the QEMU driver implements PPC64 support, this
> +         * code is related to the platform specification (PAPR), i.e. it
> +         * is hypervisor agnostic, and any future PPC64 hypervisor driver
> +         * will have the same restriction.
> +         */
> +        if (ARCH_IS_PPC64(def->os.arch) &&
> +            virDomainNVDimmAlignSizePseries(mem) < 0)
> +            return -1;
> +    }

For this and the rest of patches - shouldn't changes like this go into 
validator callback? I view post parse callbacks as "fill missing values" 
not a place to check if configuration makes sense/is valid.

Michal

On 12/1/20 3:46 PM, Michal Privoznik wrote:
> On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
>> Move 'labelsize' validation to virDomainMemoryDefPostParse().
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b1534dcc1e..5e5905f483 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -5363,15 +5363,28 @@ static int
>>   virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
>>                               const virDomainDef *def)
>>   {
>> -    /* Although only the QEMU driver implements PPC64 support, this
>> -     * code is related to the platform specification (PAPR), i.e. it
>> -     * is hypervisor agnostic, and any future PPC64 hypervisor driver
>> -     * will have the same restriction.
>> -     */
>> -    if (ARCH_IS_PPC64(def->os.arch) &&
>> -        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
>> -        virDomainNVDimmAlignSizePseries(mem) < 0)
>> -        return -1;
>> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>> +        if (mem->labelsize && mem->labelsize < 128) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("nvdimm label must be at least 128KiB"));
>> +            return -1;
>> +        }
>> +
>> +        if (mem->labelsize >= mem->size) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("label size must be smaller than NVDIMM size"));
>> +            return -1;
>> +        }
>> +
>> +        /* Although only the QEMU driver implements PPC64 support, this
>> +         * code is related to the platform specification (PAPR), i.e. it
>> +         * is hypervisor agnostic, and any future PPC64 hypervisor driver
>> +         * will have the same restriction.
>> +         */
>> +        if (ARCH_IS_PPC64(def->os.arch) &&
>> +            virDomainNVDimmAlignSizePseries(mem) < 0)
>> +            return -1;
>> +    }
> 
> For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.

You mean these callbacks?

---- domain_conf.h ----

     /* validation callbacks */
     virDomainDefValidateCallback domainValidateCallback;
     virDomainDeviceDefValidateCallback deviceValidateCallback;


These callbacks makes sense for driver specific validations, but for what
we're doing here is driver agnostic (well, most of it anyway - probably
there are QEMU specific stuff mixed in).

If we move this logic to say qemuValidateDomainDeviceDef(), then we'll need
to compensate the other drivers that won't have access to these validations
(git grep tells me it's bhyve and vz_driver). Granted, we can put these in
an unique function and use them in the callback for all the drivers, if that's
the case.


Thanks,


DHB

> 
> Michal
> 

On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/1/20 3:46 PM, Michal Privoznik wrote:
>> On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
>>> Move 'labelsize' validation to virDomainMemoryDefPostParse().
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
>>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index b1534dcc1e..5e5905f483 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -5363,15 +5363,28 @@ static int
>>>   virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
>>>                               const virDomainDef *def)
>>>   {
>>> -    /* Although only the QEMU driver implements PPC64 support, this
>>> -     * code is related to the platform specification (PAPR), i.e. it
>>> -     * is hypervisor agnostic, and any future PPC64 hypervisor driver
>>> -     * will have the same restriction.
>>> -     */
>>> -    if (ARCH_IS_PPC64(def->os.arch) &&
>>> -        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
>>> -        virDomainNVDimmAlignSizePseries(mem) < 0)
>>> -        return -1;
>>> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>>> +        if (mem->labelsize && mem->labelsize < 128) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("nvdimm label must be at least 128KiB"));
>>> +            return -1;
>>> +        }
>>> +
>>> +        if (mem->labelsize >= mem->size) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("label size must be smaller than NVDIMM 
>>> size"));
>>> +            return -1;
>>> +        }
>>> +
>>> +        /* Although only the QEMU driver implements PPC64 support, this
>>> +         * code is related to the platform specification (PAPR), 
>>> i.e. it
>>> +         * is hypervisor agnostic, and any future PPC64 hypervisor 
>>> driver
>>> +         * will have the same restriction.
>>> +         */
>>> +        if (ARCH_IS_PPC64(def->os.arch) &&
>>> +            virDomainNVDimmAlignSizePseries(mem) < 0)
>>> +            return -1;
>>> +    }
>>
>> For this and the rest of patches - shouldn't changes like this go into 
>> validator callback? I view post parse callbacks as "fill missing 
>> values" not a place to check if configuration makes sense/is valid.
> 
> You mean these callbacks?
> 
> ---- domain_conf.h ----
> 
>      /* validation callbacks */
>      virDomainDefValidateCallback domainValidateCallback;
>      virDomainDeviceDefValidateCallback deviceValidateCallback;


I mean virDomainDefValidate() and more specifically 
virDomainDefValidateInternal(). Driver specific callbacks are out of 
question - exactly for the reason you pointed out.

Michal

On 12/1/20 5:20 PM, Michal Privoznik wrote:
> On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 12/1/20 3:46 PM, Michal Privoznik wrote:
>>> On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
>>>> Move 'labelsize' validation to virDomainMemoryDefPostParse().
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>   src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
>>>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index b1534dcc1e..5e5905f483 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -5363,15 +5363,28 @@ static int
>>>>   virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
>>>>                               const virDomainDef *def)
>>>>   {
>>>> -    /* Although only the QEMU driver implements PPC64 support, this
>>>> -     * code is related to the platform specification (PAPR), i.e. it
>>>> -     * is hypervisor agnostic, and any future PPC64 hypervisor driver
>>>> -     * will have the same restriction.
>>>> -     */
>>>> -    if (ARCH_IS_PPC64(def->os.arch) &&
>>>> -        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
>>>> -        virDomainNVDimmAlignSizePseries(mem) < 0)
>>>> -        return -1;
>>>> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>>>> +        if (mem->labelsize && mem->labelsize < 128) {
>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>> +                           _("nvdimm label must be at least 128KiB"));
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        if (mem->labelsize >= mem->size) {
>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>> +                           _("label size must be smaller than NVDIMM size"));
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        /* Although only the QEMU driver implements PPC64 support, this
>>>> +         * code is related to the platform specification (PAPR), i.e. it
>>>> +         * is hypervisor agnostic, and any future PPC64 hypervisor driver
>>>> +         * will have the same restriction.
>>>> +         */
>>>> +        if (ARCH_IS_PPC64(def->os.arch) &&
>>>> +            virDomainNVDimmAlignSizePseries(mem) < 0)
>>>> +            return -1;
>>>> +    }
>>>
>>> For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
>>
>> You mean these callbacks?
>>
>> ---- domain_conf.h ----
>>
>>      /* validation callbacks */
>>      virDomainDefValidateCallback domainValidateCallback;
>>      virDomainDeviceDefValidateCallback deviceValidateCallback;
> 
> 
> I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out.

Got it. I'll not overload the PostParse() functions and, instead, use
virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal()
for these cases.

Let's try it again in v2.


Thanks,


DHB

> 
> Michal
> 

On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/1/20 5:20 PM, Michal Privoznik wrote:
>> On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 12/1/20 3:46 PM, Michal Privoznik wrote:
>>>> On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
>>>>> Move 'labelsize' validation to virDomainMemoryDefPostParse().
>>>>>
>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>> ---
>>>>>   src/conf/domain_conf.c | 43 
>>>>> +++++++++++++++++++++---------------------
>>>>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>>> index b1534dcc1e..5e5905f483 100644
>>>>> --- a/src/conf/domain_conf.c
>>>>> +++ b/src/conf/domain_conf.c
>>>>> @@ -5363,15 +5363,28 @@ static int
>>>>>   virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
>>>>>                               const virDomainDef *def)
>>>>>   {
>>>>> -    /* Although only the QEMU driver implements PPC64 support, this
>>>>> -     * code is related to the platform specification (PAPR), i.e. it
>>>>> -     * is hypervisor agnostic, and any future PPC64 hypervisor driver
>>>>> -     * will have the same restriction.
>>>>> -     */
>>>>> -    if (ARCH_IS_PPC64(def->os.arch) &&
>>>>> -        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
>>>>> -        virDomainNVDimmAlignSizePseries(mem) < 0)
>>>>> -        return -1;
>>>>> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>>>>> +        if (mem->labelsize && mem->labelsize < 128) {
>>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>>> +                           _("nvdimm label must be at least 
>>>>> 128KiB"));
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        if (mem->labelsize >= mem->size) {
>>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>>> +                           _("label size must be smaller than 
>>>>> NVDIMM size"));
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        /* Although only the QEMU driver implements PPC64 support, 
>>>>> this
>>>>> +         * code is related to the platform specification (PAPR), 
>>>>> i.e. it
>>>>> +         * is hypervisor agnostic, and any future PPC64 hypervisor 
>>>>> driver
>>>>> +         * will have the same restriction.
>>>>> +         */
>>>>> +        if (ARCH_IS_PPC64(def->os.arch) &&
>>>>> +            virDomainNVDimmAlignSizePseries(mem) < 0)
>>>>> +            return -1;
>>>>> +    }
>>>>
>>>> For this and the rest of patches - shouldn't changes like this go 
>>>> into validator callback? I view post parse callbacks as "fill 
>>>> missing values" not a place to check if configuration makes sense/is 
>>>> valid.
>>>
>>> You mean these callbacks?
>>>
>>> ---- domain_conf.h ----
>>>
>>>      /* validation callbacks */
>>>      virDomainDefValidateCallback domainValidateCallback;
>>>      virDomainDeviceDefValidateCallback deviceValidateCallback;
>>
>>
>> I mean virDomainDefValidate() and more specifically 
>> virDomainDefValidateInternal(). Driver specific callbacks are out of 
>> question - exactly for the reason you pointed out.
> 
> Got it. I'll not overload the PostParse() functions and, instead, use
> virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal()
> for these cases.
> 
> Let's try it again in v2.

Yeah, you can merge those cleanup patches to which I replied with my 
reviewed-by.

I vaguely recall that I might merge some patches of your that did 
something similar - moved checks from parser to post parse, do you 
remember? If so, I'm sorry that I misled you.

Michal

On 12/1/20 6:16 PM, Michal Privoznik wrote:
> On 12/1/20 10:03 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 12/1/20 5:20 PM, Michal Privoznik wrote:
>>> On 12/1/20 8:58 PM, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 12/1/20 3:46 PM, Michal Privoznik wrote:
>>>>> On 11/24/20 8:20 PM, Daniel Henrique Barboza wrote:
>>>>>> Move 'labelsize' validation to virDomainMemoryDefPostParse().
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>>>> ---
>>>>>>   src/conf/domain_conf.c | 43 +++++++++++++++++++++---------------------
>>>>>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>>>>>

[...]

>>>>>> +        /* Although only the QEMU driver implements PPC64 support, this
>>>>>> +         * code is related to the platform specification (PAPR), i.e. it
>>>>>> +         * is hypervisor agnostic, and any future PPC64 hypervisor driver
>>>>>> +         * will have the same restriction.
>>>>>> +         */
>>>>>> +        if (ARCH_IS_PPC64(def->os.arch) &&
>>>>>> +            virDomainNVDimmAlignSizePseries(mem) < 0)
>>>>>> +            return -1;
>>>>>> +    }
>>>>>
>>>>> For this and the rest of patches - shouldn't changes like this go into validator callback? I view post parse callbacks as "fill missing values" not a place to check if configuration makes sense/is valid.
>>>>
>>>> You mean these callbacks?
>>>>
>>>> ---- domain_conf.h ----
>>>>
>>>>      /* validation callbacks */
>>>>      virDomainDefValidateCallback domainValidateCallback;
>>>>      virDomainDeviceDefValidateCallback deviceValidateCallback;
>>>
>>>
>>> I mean virDomainDefValidate() and more specifically virDomainDefValidateInternal(). Driver specific callbacks are out of question - exactly for the reason you pointed out.
>>
>> Got it. I'll not overload the PostParse() functions and, instead, use
>> virDomainDefValidateInternal() and virDomainDeviceDefValidateInternal()
>> for these cases.
>>
>> Let's try it again in v2.
> 
> Yeah, you can merge those cleanup patches to which I replied with my reviewed-by.

Just did. Thanks for the reviews!

> 
> I vaguely recall that I might merge some patches of your that did something similar - moved checks from parser to post parse, do you remember? If so, I'm sorry that I misled you.


Nah don't worry about it. It's all learning experience. Besides, the only
one instance I'm recalling doing that ATM is with a NVDIMM code that wasn't
you who merged.

(and this particular code can be moved to the proper place like we discussed
above. I'll keep that in mind when sending the v2).



Thanks,

DHB

> 
> Michal
>