[libvirt] [PATCHv4 6/9] conf: Enforce ranges on cputune variables

[Laine Stump]

On 03/15/2013 11:26 AM, Peter Krempa wrote:
> The limits are documented at
> http://libvirt.org/formatdomain.html#elementsCPUTuning . Enforce them
> when going through XML parsing in addition to being enforced by the API.

What's the rationale for doing this validation during the post-parse
rather than just doing it as the cputune elements are being parsed? They
don't depend on any device that might be modified during a post-parse
callback (or any other unrelated part of the domain). My opinion is that
a separate post-parse validation should only be done if:

1) the validation depends on hypervisor (in which case it will be done
in a hypervisor-specific callback)

2) the validation depends on some other element of the domain object (in
which case it would be done in virDomainDefPostParseInternal, as you've
done here)

or

2a) the validation depends on some other element of the domain that
could be changed by a hypervisor-specific post-parse validation function.

Doing it in a separate function when none of the above is true just has
the effect of spreading out the parsing of a single element into
multiple places, making it more difficult to understand and maintain the
code.

> ---
>
> Notes:
>     Version 4:
>     - changed error from VIR_ERR_XML_ERROR to VIR_ERR_CONFIG_UNSUPPORTED
>     Version 3:
>     - new in series
>
>  src/conf/domain_conf.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fde88b2..5a59e3f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2499,6 +2499,43 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>          return -1;
>      }
>
> +    /* enforce range checks for cputune values */
> +    /* these are not represented in the XML schema, but are documented */
> +    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.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.quota > 0 &&
> +        (def->cputune.quota < 1000 ||
> +         def->cputune.quota > 18446744073709551)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Value of cputune quota must be in range "
> +                         "[1000, 18446744073709551]"));
> +        return -1;
> +    }
> +
> +    if (def->cputune.emulator_quota > 0 &&
> +        (def->cputune.emulator_quota < 1000 ||
> +         def->cputune.emulator_quota > 18446744073709551)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Value of cputune emulator_quota must be in range "
> +                         "[1000, 18446744073709551]"));
> +        return -1;
> +    }
> +
>      return 0;
>  }
>