[PATCH v2] conf: Move validation from virDomainDiskDefIotuneParse into the validation callback

[Peter Krempa]

On Thu, Apr 07, 2022 at 22:57:29 +0530, Moteen Shah wrote:
> From: Moteen Shah <moteenshah.02 at gmail.com>
> 
> all the option collision total and... error messages in
> virDomainDiskDefIotuneParse shifted to validation callback
> 
> Signed-off-by: Moteen Shah <moteenshah.02 at gmail.com>
> ---
>  src/conf/domain_conf.c     | 55 +++-----------------------------------
>  src/conf/domain_validate.c | 42 ++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 52 deletions(-)

Firstly this patch doesn't apply to current master even with 3 way merge,
please rebase it before sending the next time.


> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f2480f37f6..48132c2f97 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1408,7 +1408,6 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation,
>                "no",
>                "internal",
>                "external",
> -              "manual",
>  );


This looks like an improperly fixed rebase conflict and must be removed
as it would break other code. In fact the tree wouldn't even compile
with that change as VIR_ENUM_IMPL has a compile time check that all
values of the enum are present in the implementation macro.


>  
>  /* Internal mapping: subset of block job types that can be present in
> @@ -6229,51 +6228,6 @@ virDomainDefPostParseCheckFailure(virDomainDef *def,
>      return 0;
>  }
>  
> -static int
> -virDomainDefPostParseCheck(virDomainDiskDef *def)
> -{
> -    if ((def->blkdeviotune.total_bytes_sec &&
> -         def->blkdeviotune.read_bytes_sec) ||
> -        (def->blkdeviotune.total_bytes_sec &&
> -         def->blkdeviotune.write_bytes_sec)) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("total and read/write bytes_sec "
> -                         "cannot be set at the same time"));
> -        return -1;
> -    }
> -
> -    if ((def->blkdeviotune.total_iops_sec &&
> -         def->blkdeviotune.read_iops_sec) ||
> -        (def->blkdeviotune.total_iops_sec &&
> -         def->blkdeviotune.write_iops_sec)) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("total and read/write iops_sec "
> -                         "cannot be set at the same time"));
> -        return -1;
> -    }
> -
> -    if ((def->blkdeviotune.total_bytes_sec_max &&
> -         def->blkdeviotune.read_bytes_sec_max) ||
> -        (def->blkdeviotune.total_bytes_sec_max &&
> -         def->blkdeviotune.write_bytes_sec_max)) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("total and read/write bytes_sec_max "
> -                         "cannot be set at the same time"));
> -        return -1;
> -    }
> -
> -    if ((def->blkdeviotune.total_iops_sec_max &&
> -         def->blkdeviotune.read_iops_sec_max) ||
> -        (def->blkdeviotune.total_iops_sec_max &&
> -         def->blkdeviotune.write_iops_sec_max)) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("total and read/write iops_sec_max "
> -                         "cannot be set at the same time"));
> -        return -1;
> -    }
> -    return 0;

Is this patch against your previous patch, because this function is not
present in the current upstream tree.

Either way, if modifications are requested for a patch you should update
the patch to do so, and not layer fixes on top of other fixes.

Namely the problem is that in most cases the tree won't build cleanly
and we require that all patches build successfully for bisection
reasons. See:

https://libvirt.org/hacking.html#preparing-patches

> -}
> -
>  int
>  virDomainDefPostParse(virDomainDef *def,
>                        unsigned int parseFlags,
> @@ -8826,9 +8780,9 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
>      PARSE_IOTUNE(write_iops_sec_max_length);
>  
>      def->blkdeviotune.group_name =
> -        virXPathString("string(./iotune/group_name)", ctxt);
> -
> -    return virDomainDefPostParseCheck(def);
> +    virXPathString("string(./iotune/group_name)", ctxt);
> +    

This whitespace change is unwanted.

> +    return 0;
>  }
>  #undef PARSE_IOTUNE
>  
> @@ -9278,8 +9232,7 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
>              return NULL;
>      }
>  
> -    if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
> -        return NULL;
> +    virDomainDiskDefIotuneParse(def, ctxt);

This is certainly wrong the PARSE_IOTUNE macro used in virDomainDiskDefIotuneParse
can return -1 and thus you must keep the return value check intact.

>  
>      def->domain_name = virXPathString("string(./backenddomain/@name)", ctxt);
>      def->serial = virXPathString("string(./serial)", ctxt);
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index d6869e8fd8..4bdb95aa13 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -597,7 +597,6 @@ virDomainDiskDefSourceLUNValidate(const virStorageSource *src)
>      return 0;
>  }
>  
> -

Another spurious whitespace change.

>  static int
>  virDomainDiskDefValidate(const virDomainDef *def,
>                           const virDomainDiskDef *disk)
> @@ -651,6 +650,47 @@ virDomainDiskDefValidate(const virDomainDef *def,
>          }
>      }
>  
> +    /* Validate IotuneParse */
> +    if ((disk->blkdeviotune.total_bytes_sec &&
> +         disk->blkdeviotune.read_bytes_sec) ||
> +        (disk->blkdeviotune.total_bytes_sec &&
> +         disk->blkdeviotune.write_bytes_sec)) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("total and read/write bytes_sec "
> +                         "cannot be set at the same time"));
> +        return -1;
> +    }
> +
> +    if ((disk->blkdeviotune.total_iops_sec &&
> +         disk->blkdeviotune.read_iops_sec) ||
> +        (disk->blkdeviotune.total_iops_sec &&
> +         disk->blkdeviotune.write_iops_sec)) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("total and read/write iops_sec "
> +                         "cannot be set at the same time"));
> +        return -1;
> +    }
> +
> +    if ((disk->blkdeviotune.total_bytes_sec_max &&
> +         disk->blkdeviotune.read_bytes_sec_max) ||
> +        (disk->blkdeviotune.total_bytes_sec_max &&
> +         disk->blkdeviotune.write_bytes_sec_max)) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("total and read/write bytes_sec_max "
> +                         "cannot be set at the same time"));
> +        return -1;
> +    }
> +
> +    if ((disk->blkdeviotune.total_iops_sec_max &&
> +         disk->blkdeviotune.read_iops_sec_max) ||
> +        (disk->blkdeviotune.total_iops_sec_max &&
> +         disk->blkdeviotune.write_iops_sec_max)) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("total and read/write iops_sec_max "
> +                         "cannot be set at the same time"));
> +        return -1;
> +    }
> +
>      /* Reject disks with a bus type that is not compatible with the
>       * given address type. The function considers only buses that are
>       * handled in common code. For other bus types it's not possible
> -- 
> 2.35.1
>