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

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(a)gmail.com&gt; >>>>> --- >>>>>   src/conf/domain_conf.c | 43 +++++++++++++++++++++--------------------- >>>>>   1 file changed, 22 insertions(+), 21 deletions(-) >>>>>

>>>>> +        /* 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