Re: [PATCH v2 27/29] conf: Refactor virDomainDiskDefMirrorParse

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Mar 22, 2019 at 07:01:03PM +0100, Peter Krempa wrote:
> Use virDomainStorageSourceParseBase and other tricks.

Would be nicer to read with the virDomainStorageSourceParseBase trick
and the virDomainStorageSourceParse trick being separate.

> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 52 +++++++++++++-----------------------------
> 1 file changed, 16 insertions(+), 36 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 40924b1d29..2514c60744 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9315,14 +9315,13 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
>                             unsigned int flags,
>                             virDomainXMLOptionPtr xmlopt)
> {
> -    xmlNodePtr mirrorNode;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);
>     VIR_AUTOFREE(char *) mirrorFormat = NULL;
>     VIR_AUTOFREE(char *) mirrorType = NULL;
>     VIR_AUTOFREE(char *) ready = NULL;
>     VIR_AUTOFREE(char *) blockJob = NULL;
>
> -    if (!(def->mirror = virStorageSourceNew()))
> -        return -1;
> +    ctxt->node = cur;
>
>     if ((blockJob = virXMLPropString(cur, "job"))) {
>         if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) {
> @@ -9335,35 +9334,8 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
>     }
>
>     if ((mirrorType = virXMLPropString(cur, "type"))) {
> -        if ((def->mirror->type = virStorageTypeFromString(mirrorType)) <= 0) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("unknown mirror backing store type '%s'"),
> -                           mirrorType);
> -            return -1;
> -        }
> -
> -        mirrorFormat = virXPathString("string(./mirror/format/@type)", ctxt);
> -

> -        if (!(mirrorNode = virXPathNode("./mirror/source", ctxt))) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("mirror requires source element"));
> -            return -1;
> -        }
> -

I'd move this

> -        if (virDomainStorageSourceParse(mirrorNode, ctxt, def->mirror,
> -                                        flags, xmlopt) < 0)
> -            return -1;
> +        mirrorFormat = virXPathString("string(./format/@type)", ctxt);
>     } else {
> -        /* For back-compat reasons, we handle a file name
> -         * encoded as attributes, even though we prefer
> -         * modern output in the style of backingStore */
> -        def->mirror->type = VIR_STORAGE_TYPE_FILE;
> -        def->mirror->path = virXMLPropString(cur, "file");
> -        if (!def->mirror->path) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("mirror requires file name"));
> -            return -1;
> -        }
>         if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
>             virReportError(VIR_ERR_XML_ERROR, "%s",
>                            _("mirror without type only supported "
> @@ -9373,11 +9345,19 @@ virDomainDiskDefMirrorParse(virDomainDiskDefPtr def,
>         mirrorFormat = virXMLPropString(cur, "format");
>     }
>
> -    if (mirrorFormat) {
> -        def->mirror->format = virStorageFileFormatTypeFromString(mirrorFormat);
> -        if (def->mirror->format <= 0) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("unknown mirror format value '%s'"), mirrorFormat);
> +    if (!(def->mirror = virDomainStorageSourceParseBase(mirrorType, mirrorFormat, NULL)))
> +        return -1;
> +
> +    if (mirrorType) {

here.

> +        if (virDomainStorageSourceParse(NULL, ctxt, def->mirror, flags, xmlopt) < 0)
> +            return -1;
> +    } else {
> +        /* For back-compat reasons, we handle a file name encoded as
> +         * attributes, even though we prefer modern output in the style of
> +         * backingStore */
> +        if (!(def->mirror->path = virXMLPropString(cur, "file"))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("mirror requires file name"));
>             return -1;

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list