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

[Michal Privoznik]

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.

Michal