[PATCH 01/21] domain_conf.c: move NVDIMM 'labelsize' check to post parse

[Daniel Henrique Barboza]

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 at 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
>