[PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c

[Michal Privoznik]

On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote:
> This patch creates a new function, virDomainDefBootValidate(), to host
> the validation of boot menu timeout and rebootTimeout outside of parse
> time. The checks in virDomainDefParseBootXML() were changed to throw
> VIR_ERR_XML_ERROR in case of parse error of those values.
> 
> In an attempt to alleviate the amount of code being stacked inside
> domain_conf.c, let's put this new function in a new domain_validate.c
> file that will be used to place these validations.
> 
> Suggested-by: Michal Privoznik <mprivozn at redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>   po/POTFILES.in             |  1 +
>   src/conf/domain_conf.c     | 20 +++++++--------
>   src/conf/domain_validate.c | 51 ++++++++++++++++++++++++++++++++++++++
>   src/conf/domain_validate.h | 27 ++++++++++++++++++++
>   src/conf/meson.build       |  1 +
>   tests/qemuxml2argvtest.c   |  3 ++-
>   6 files changed, 92 insertions(+), 11 deletions(-)
>   create mode 100644 src/conf/domain_validate.c
>   create mode 100644 src/conf/domain_validate.h
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 9782b2beb4..14636d4b93 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -26,6 +26,7 @@
>   @SRCDIR at src/conf/domain_capabilities.c
>   @SRCDIR at src/conf/domain_conf.c
>   @SRCDIR at src/conf/domain_event.c
> + at SRCDIR@src/conf/domain_validate.c
>   @SRCDIR at src/conf/interface_conf.c
>   @SRCDIR at src/conf/netdev_bandwidth_conf.c
>   @SRCDIR at src/conf/netdev_vlan_conf.c
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66ee658e7b..4a45c76cf1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -33,6 +33,7 @@
>   #include "datatypes.h"
>   #include "domain_addr.h"
>   #include "domain_conf.h"
> +#include "domain_validate.h"
>   #include "snapshot_conf.h"
>   #include "viralloc.h"
>   #include "virxml.h"
> @@ -7344,6 +7345,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
>       if (virDomainDefCputuneValidate(def) < 0)
>           return -1;
>   
> +    if (virDomainDefBootValidate(def) < 0)
> +        return -1;
> +
>       if (virDomainNumaDefValidate(def->numa) < 0)
>           return -1;
>   
> @@ -18867,11 +18871,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
>   
>           tmp = virXMLPropString(node, "timeout");
>           if (tmp && def->os.bootmenu == VIR_TRISTATE_BOOL_YES) {
> -            if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0 ||
> -                def->os.bm_timeout > 65535) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("invalid value for boot menu timeout, "
> -                                 "must be in range [0,65535]"));
> +            if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("invalid value for boot menu timeout"));
>                   return -1;
>               }
>               def->os.bm_timeout_set = true;
> @@ -18892,11 +18894,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
>           if (tmp) {
>               /* that was really just for the check if it is there */
>   
> -            if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0 ||
> -                def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("invalid value for rebootTimeout, "
> -                                 "must be in range [-1,65535]"));
> +            if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("invalid value for rebootTimeout"));
>                   return -1;
>               }
>               def->os.bios.rt_set = true;
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> new file mode 100644
> index 0000000000..e4d947c553
> --- /dev/null
> +++ b/src/conf/domain_validate.c
> @@ -0,0 +1,51 @@
> +/*
> + * domain_validate.c: domain general validation functions
> + *
> + * Copyright IBM Corp, 2020

Honestly, I have only vague idea how these Copyright lines work, but 
shouldn't they also include (at least subset) of the lines from the 
original file? I mean, my common sense tells me that if I have a file 
written by person X, and later the file is split into two the person X 
is still the original author. Extending that, if a company holds a 
copyright on a file then moving bits out to a different file should keep 
the copyright. But I admit that law has completely different model of 
"common sense". And also there is a disconnection between files and 
these Copyright lines. If a copyright holder Y changed a tiny bit that 
is not moved - should their Copyright line also appear in the new file?

Michal