[libvirt] [PATCH v7 14/23] backup: Parse and output backup XML

[Peter Krempa]

On Wed, Mar 27, 2019 at 05:10:45 -0500, Eric Blake wrote:
> Accept XML describing a generic block job, and output it again as
> needed. This may still need a few tweaks to match the documented XML
> and RNG schema.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/backup_conf.h   |  96 +++++++
>  src/conf/virconftypes.h  |   3 +
>  src/conf/Makefile.inc.am |   2 +
>  src/conf/backup_conf.c   | 544 +++++++++++++++++++++++++++++++++++++++
>  src/libvirt_private.syms |   8 +-
>  5 files changed, 652 insertions(+), 1 deletion(-)
>  create mode 100644 src/conf/backup_conf.h
>  create mode 100644 src/conf/backup_conf.c

[...]

> +static int
> +virDomainBackupDiskDefParseXML(xmlNodePtr node,
> +                               xmlXPathContextPtr ctxt,
> +                               virDomainBackupDiskDefPtr def,
> +                               bool push, bool internal,
> +                               virDomainXMLOptionPtr xmlopt)
> +{
> +    int ret = -1;
> +    //    char *backup = NULL; /* backup="yes|no"? */
> +    char *type = NULL;
> +    char *driver = NULL;
> +    xmlNodePtr cur;
> +    xmlNodePtr saved = ctxt->node;
> +
> +    ctxt->node = node;
> +
> +    if (VIR_ALLOC(def->store) < 0)
> +        goto cleanup;
> +
> +    def->name = virXMLPropString(node, "name");
> +    if (!def->name) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("missing name from disk backup element"));
> +        goto cleanup;
> +    }
> +
> +    /* Needed? A way for users to list a disk and explicitly mark it
> +     * as not participating, and then output shows all disks rather
> +     * than just active disks */
> +#if 0
> +    backup = virXMLPropString(node, "backup");
> +    if (backup) {
> +        def->type = virDomainCheckpointTypeFromString(checkpoint);
> +        if (def->type <= 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown disk checkpoint setting '%s'"),
> +                           checkpoint);
> +            goto cleanup;
> +        }
> +    }
> +#endif

Some leftovers?

> +
> +    if ((type = virXMLPropString(node, "type"))) {
> +        if ((def->store->type = virStorageTypeFromString(type)) <= 0 ||
> +            def->store->type == VIR_STORAGE_TYPE_VOLUME ||
> +            def->store->type == VIR_STORAGE_TYPE_DIR) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("unknown disk backup type '%s'"), type);
> +            goto cleanup;
> +        }
> +    } else {
> +        def->store->type = VIR_STORAGE_TYPE_FILE;
> +    }
> +
> +    if ((cur = virXPathNode(push ? "./target" : "./scratch", ctxt)) &&
> +        virDomainDiskSourceParse(cur, ctxt, def->store, 0, xmlopt) < 0)
> +        goto cleanup;
> +
> +    if (internal) {
> +        int detected;
> +        if (virXPathInt("string(./node/@detected)", ctxt, &detected) < 0)
> +            goto cleanup;
> +        def->store->detected = detected;
> +        def->store->nodeformat = virXPathString("string(./node)", ctxt);

This should not be public name. Current design of nodenames does not
allow you to depend on the name.

In general, nodenames are a qemu-specific thing thus should not be
expressed nor documented in the public XML.

Same applies for the formatter below.

(yes, the original implementation is suboptimal as I've put
nodeformat/nodestorage into virStorageSource, but for domain stuff it's
never formatted using the code in src/conf)

> +    }
> +
> +    if ((driver = virXPathString("string(./driver/@type)", ctxt))) {
> +        def->store->format = virStorageFileFormatTypeFromString(driver);
> +        if (def->store->format <= 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown disk backup driver '%s'"), driver);
> +            goto cleanup;
> +        } else if (!push && def->store->format != VIR_STORAGE_FILE_QCOW2) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("pull mode requires qcow2 driver, not '%s'"),
> +                           driver);
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* validate that the passed path is absolute */
> +    if (virStorageSourceIsRelative(def->store)) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("disk backup image path '%s' must be absolute"),
> +                       def->store->path);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    ctxt->node = saved;
> +
> +    VIR_FREE(driver);
> +//    VIR_FREE(backup);

here too

> +    VIR_FREE(type);
> +    if (ret < 0)
> +        virDomainBackupDiskDefClear(def);
> +    return ret;
> +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190327/9f8e42d3/attachment-0001.sig>