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

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(a)gmail.com&gt; >>>> --- >>>>   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.