conf: Introduce virDomainDefCputuneValidate helper ...would probably sound better... When sending further revisions of a patch it should be noted in the subject prefix, you can do that by using '-vN' when formatting patches with git. On Tue, Mar 12, 2019 at 03:32:40PM -0600, Suyang Chen wrote: > move all the def->cputune 'period must be in range' errors into > virDomainDefCputuneValidate function and have it called from > virDomainDefValidateInternal and virDomainDefParseXML function So, it should only be either of those two, not both, so the call in virDomainDefParseXML has to go, otherwise we'll validate twice which is pointless and we didn't really improve anything. > > Solve the bitsizedtask: > "Move validation checks out of domain XML parsing" > > Resolves: > https://wiki.libvirt.org/page/BiteSizedTasks#Move_validation_checks_out_of_domain_XML_parsing There are a few more issues, so I wouldn't say we resolved it just yet, I'm not saying you have to fix all of them (it'd be cool though), you can try others as well, but this one doesn't get resolved solely with this single patch. > > Signed-off-by: Suyang Chen <dawson0xff@xxxxxxxxx> > --- > src/conf/domain_conf.c | 75 ++++++++++++++++++++++++------------------ > 1 file changed, 43 insertions(+), 32 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 995f87bcbe..e17ca0e0cb 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -6589,6 +6589,45 @@ virDomainDefMemtuneValidate(const virDomainDef *def) > return 0; > } > > +static int > +virDomainDefCputuneValidate(const virDomainDef *def) > +{ > + if (def->cputune.period > 0 && > + (def->cputune.period < 1000 || def->cputune.period > 1000000)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Value of cputune period must be in range " > + "[1000, 1000000]")); > + return -1; > + } > + > + if (def->cputune.global_period > 0 && > + (def->cputune.global_period < 1000 || def->cputune.global_period > 1000000)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Value of cputune global period must be in range " > + "[1000, 1000000]")); > + return -1; > + } > + > + if (def->cputune.emulator_period > 0 && > + (def->cputune.emulator_period < 1000 || > + def->cputune.emulator_period > 1000000)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Value of cputune emulator_period must be in range " > + "[1000, 1000000]")); > + return -1; > + } > + > + if (def->cputune.iothread_period > 0 && > + (def->cputune.iothread_period < 1000 || > + def->cputune.iothread_period > 1000000)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Value of cputune iothread_period must be in range " > + "[1000, 1000000]")); > + return -1; > + } So, all of ^these share certain similarities, so we can consolidate all of the checks into a single macro which would be called over the struct members, IOW something like: #define CPUTUNE_VALIDATE_PERIOD(name) ... CPUTUNE_VALIDATE_PERIOD(period); ... #undef CPUTUNE_VALIDATE_MACRO // don't forget <<this undef afterwards ;) Looking at the quotas, you can define a macro that is going to be able to handle both periods and quotas. > + > + return 0; > +} > > static int > virDomainDefValidateInternal(const virDomainDef *def) > @@ -6628,6 +6667,9 @@ virDomainDefValidateInternal(const virDomainDef *def) > if (virDomainDefMemtuneValidate(def) < 0) > return -1; > > + if (virDomainDefCputuneValidate(def) < 0) > + return -1; > + > return 0; > } > > @@ -19594,13 +19636,8 @@ virDomainDefParseXML(xmlDocPtr xml, > goto error; > } > > - if (def->cputune.period > 0 && > - (def->cputune.period < 1000 || def->cputune.period > 1000000)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Value of cputune period must be in range " > - "[1000, 1000000]")); > + if (virDomainDefCputuneValidate(def) < 0) As mentioned above, ^this call needs to be dropped. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
