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

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.

Michal