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

[Michal Privoznik]

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

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