[libvirt] [PATCH] conf: Add a function virDomainDefCputuneValidate

[Erik Skultety]

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 at 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