Re: [libvirt] [PATCH] conf: Add a function virDomainDefCputuneValidate
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_o...
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(a)gmail.com>
---
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