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

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;