Re: [libvirt] [PATCH v4 12/20] wip: backup: Parse and output backup XML

12 Feb 2019

On 2/6/19 2:18 PM, Eric Blake wrote:
...
Accept XML describing a generic block job, and output it again
as needed. At the moment, it has some qemu-specific hacks, such
as storing internal XML for a node name, that might be cleaner
once full-tree node-name support goes in.
Still not done: decent tests
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/conf/checkpoint_conf.h |  65 +++++
 src/conf/domain_conf.h     |   3 +
 src/conf/checkpoint_conf.c | 503 +++++++++++++++++++++++++++++++++++++
 src/libvirt_private.syms   |   8 +-
 4 files changed, 578 insertions(+), 1 deletion(-)
I think this should be it's own module src/conf/backup_conf.{c,h}
...

diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
index 994a8bd083..abaeaaad52 100644
--- a/src/conf/checkpoint_conf.h
+++ b/src/conf/checkpoint_conf.h
@@ -147,4 +147,69 @@ int virDomainCheckpointRedefinePrep(virDomainPtr domain,
VIR_ENUM_DECL(virDomainCheckpoint);
+/* Items related to incremental backup state */
+
+typedef enum {
+    VIR_DOMAIN_BACKUP_TYPE_DEFAULT = 0,
+    VIR_DOMAIN_BACKUP_TYPE_PUSH,
+    VIR_DOMAIN_BACKUP_TYPE_PULL,
+
+    VIR_DOMAIN_BACKUP_TYPE_LAST
+} virDomainBackupType;
+
+typedef enum {
+    VIR_DOMAIN_BACKUP_DISK_STATE_DEFAULT = 0, /* Initial */
+    VIR_DOMAIN_BACKUP_DISK_STATE_CREATED, /* File created */
+    VIR_DOMAIN_BACKUP_DISK_STATE_LABEL, /* Security labels applied */
+    VIR_DOMAIN_BACKUP_DISK_STATE_READY, /* Handed to guest */
+    VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP, /* Associated temp bitmap created */
+    VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT, /* NBD export created */
+} virDomainBackupDiskState;
+
+/* Stores disk-backup information */
+typedef struct _virDomainBackupDiskDef virDomainBackupDiskDef;
+typedef virDomainBackupDiskDef *virDomainBackupDiskDefPtr;
+struct _virDomainBackupDiskDef {
+    char *name;     /* name matching the <target dev='...' of the domain */
+    int idx;        /* index within checkpoint->dom->disks that matches name */
+
+    /* details of target for push-mode, or of the scratch file for pull-mode */
+    virStorageSourcePtr store;
+    int state;      /* virDomainBackupDiskState, not stored in XML */
+};
+
+/* Stores the complete backup metadata */
+typedef struct _virDomainBackupDef virDomainBackupDef;
+typedef virDomainBackupDef *virDomainBackupDefPtr;
+struct _virDomainBackupDef {
+    /* Public XML.  */
+    int type; /* virDomainBackupType */
+    int id;
+    char *incremental;
+    virStorageNetHostDefPtr server; /* only when type == PULL */
+
+    size_t ndisks; /* should not exceed dom->ndisks */
+    virDomainBackupDiskDef *disks;
+};
Like checkpoint, the "struct _vir*" probably should go with in the .c
files and accessors created.
...
+
+VIR_ENUM_DECL(virDomainBackup);
+
+typedef enum {
+    VIR_DOMAIN_BACKUP_PARSE_INTERNAL = 1 << 0,
+} virDomainBackupParseFlags;
+
+virDomainBackupDefPtr virDomainBackupDefParseString(const char *xmlStr,
+                                                    virDomainXMLOptionPtr xmlopt,
+                                                    unsigned int flags);
+virDomainBackupDefPtr virDomainBackupDefParseNode(xmlDocPtr xml,
+                                                  xmlNodePtr root,
+                                                  virDomainXMLOptionPtr xmlopt,
+                                                  unsigned int flags);
+void virDomainBackupDefFree(virDomainBackupDefPtr def);
+int virDomainBackupDefFormat(virBufferPtr buf,
+                             virDomainBackupDefPtr def,
+                             bool internal);
+int virDomainBackupAlignDisks(virDomainBackupDefPtr backup,
+                              virDomainDefPtr dom, const char *suffix);
+
Similar to previous patch.

Could also have the

VIR_DEFINE_AUTOPTR_FUNC(virDomainBackupDef, virDomainBackupDefFree);
...
#endif /* LIBVIRT_CHECKPOINT_CONF_H */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index d31c45427e..fe818a2b27 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -125,6 +125,9 @@ typedef virDomainCheckpointObj *virDomainCheckpointObjPtr;
 typedef struct _virDomainCheckpointObjList virDomainCheckpointObjList;
 typedef virDomainCheckpointObjList *virDomainCheckpointObjListPtr;
+typedef struct _virDomainBackupDef virDomainBackupDef;
+typedef virDomainBackupDef *virDomainBackupDefPtr;
+
 typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
 typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index c0840a96b2..4b148827c1 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -1028,3 +1028,506 @@ virDomainCheckpointRedefinePrep(virDomainPtr domain,
  cleanup:
     return ret;
 }
+
+/* Backup Def functions */
+
+VIR_ENUM_IMPL(virDomainBackup, VIR_DOMAIN_BACKUP_TYPE_LAST,
+              "default", "push", "pull");
+
+static void
+virDomainBackupDiskDefClear(virDomainBackupDiskDefPtr disk)
+{
+    VIR_FREE(disk->name);
+    virStorageSourceClear(disk->store);
+    disk->store = NULL;
Initialize idx and state?
...
+}
Similar to previous w/ 2 blank lines.
...
+
+void
+virDomainBackupDefFree(virDomainBackupDefPtr def)
+{
+    size_t i;
+
+    if (!def)
+        return;
+
+    VIR_FREE(def->incremental);
+    VIR_FREE(def->server); // FIXME which struct
True...  You'll need some what to Free only what you've allocated or if
there's a generic Server*Free function to use it.
...
+    for (i = 0; i < def->ndisks; i++)
+        virDomainBackupDiskDefClear(&def->disks[i]);
+    VIR_FREE(def->disks);
+    VIR_FREE(def);
+}
+
+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;
VIR_AUTOFREE(char *) type = NULL;
    VIR_AUTOFREE(char *) driver = NULL;
...
+
+    ctxt->node = node;
+
+    if (VIR_ALLOC(def->store) < 0)
+        goto cleanup;
Rather than this you could have a @store which would stolen at the end
before cleanup if everything has passed through parsing. I'm in the
processing of adding a VIR_AUTOPTR(virStorageSource) too.
...
+
+    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
Still need to decide...
...
+
+    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) {
Is this another way of using FLAGS to determine the parse mode?  IOW, is
this only needed when parsing the status/running XML.
...
+        int detected;
+        if (virXPathInt("string(./node/@detected)", ctxt, &detected) < 0)
+            goto cleanup;
+        def->store->detected = detected;
+        def->store->nodeformat = virXPathString("string(./node)", ctxt);
+    }
+
+    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);
+    VIR_FREE(type);
VIR_FREE's won't be necessary w/ autofree stuff.
...
+    if (ret < 0)
+        virDomainBackupDiskDefClear(def);
Unnecessary since the caller will do this on error with
virDomainBackupDefFree
...
+    return ret;
+}
+
+static virDomainBackupDefPtr
+virDomainBackupDefParse(xmlXPathContextPtr ctxt,
+                        virDomainXMLOptionPtr xmlopt,
+                        unsigned int flags)
+{
+    virDomainBackupDefPtr def = NULL;
+    virDomainBackupDefPtr ret = NULL;
+    xmlNodePtr *nodes = NULL;
+    xmlNodePtr node = NULL;
+    char *mode = NULL;
+    bool push;
+    size_t i;
+    int n;
VIR_AUTOPTR(virDomainBackupDef) def = NULL;
    VIR_AUTOFREE(char *) mode = NULL;
    VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
...
+
+    if (VIR_ALLOC(def) < 0)
+        goto cleanup;
+
+    mode = virXMLPropString(ctxt->node, "mode");
+    if (mode) {
+        def->type = virDomainBackupTypeFromString(mode);
+        if (def->type <= 0) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("unknown backup mode '%s'"), mode);
+            goto cleanup;
+        }
+    } else {
+        def->type = VIR_DOMAIN_BACKUP_TYPE_PUSH;
+    }
+    push = def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH;
+
+    if (flags & VIR_DOMAIN_BACKUP_PARSE_INTERNAL) {
+        char *tmp = virXMLPropString(ctxt->node, "id");
You can use VIR_AUTOFREE(char *) tmp too...

Ahh... so this is where it's referenced... I recall this from RNG...
...
+        if (tmp && virStrToLong_i(tmp, NULL, 10, &def->id) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid 'id' value '%s'"), tmp);
+            VIR_FREE(tmp);
+            goto cleanup;
+        }
+        VIR_FREE(tmp);
+    }
+
+    def->incremental = virXPathString("string(./incremental)", ctxt);
+
+    node = virXPathNode("./server", ctxt);
+    if (node) {
+        if (def->type != VIR_DOMAIN_BACKUP_TYPE_PULL) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("use of <server> requires pull mode backup"));
+            goto cleanup;
+        }
+        if (VIR_ALLOC(def->server) < 0)
+            goto cleanup;
+        if (virDomainStorageNetworkParseHost(node, def->server) < 0)
+            goto cleanup;
+        if (def->server->transport == VIR_STORAGE_NET_HOST_TRANS_RDMA) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("transport rdma is not supported for <server>"));
If transport == FOO is ever added and not supported, then you're in
trouble... Go with

    if (def->server->transport != VIR_STORAGE_NET_HOST_TRANS_...)
{TCP|UNIX}

and use the virStorageHostNetTransportTypeToString to translate what is
not supported...
...
+            goto cleanup;
+        }
+    }
+
+    if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
+        goto cleanup;
+    if (n && VIR_ALLOC_N(def->disks, n) < 0)
+        goto cleanup;
+    def->ndisks = n;
+    for (i = 0; i < def->ndisks; i++) {
+        if (virDomainBackupDiskDefParseXML(nodes[i], ctxt,
+                                           &def->disks[i], push,
+                                           flags & VIR_DOMAIN_BACKUP_PARSE_INTERNAL,
+                                           xmlopt) < 0)
+            goto cleanup;
+    }
+    VIR_FREE(nodes);
+
+    VIR_STEAL_PTR(ret, def);
+
+ cleanup:
+    VIR_FREE(mode);
+    VIR_FREE(nodes);
+    virDomainBackupDefFree(def);
Autofree removes the need for cleanup here, so we could return NULL
directly on failure paths.
...
+
+    return ret;
+}
+
+virDomainBackupDefPtr
+virDomainBackupDefParseString(const char *xmlStr,
+                              virDomainXMLOptionPtr xmlopt,
+                              unsigned int flags)
+{
+    virDomainBackupDefPtr ret = NULL;
+    xmlDocPtr xml;
+    int keepBlanksDefault = xmlKeepBlanksDefault(0);
+
+    if ((xml = virXMLParse(NULL, xmlStr, _("(domain_backup)")))) {
+        xmlKeepBlanksDefault(keepBlanksDefault);
+        ret = virDomainBackupDefParseNode(xml, xmlDocGetRootElement(xml),
+                                          xmlopt, flags);
+        xmlFreeDoc(xml);
+    }
+    xmlKeepBlanksDefault(keepBlanksDefault);
+
+    return ret;
+}
+
+virDomainBackupDefPtr
+virDomainBackupDefParseNode(xmlDocPtr xml,
+                            xmlNodePtr root,
+                            virDomainXMLOptionPtr xmlopt,
+                            unsigned int flags)
+{
+    xmlXPathContextPtr ctxt = NULL;
+    virDomainBackupDefPtr def = NULL;
+
+    if (!virXMLNodeNameEqual(root, "domainbackup")) {
+        virReportError(VIR_ERR_XML_ERROR, "%s", _("domainbackup"));
+        goto cleanup;
return NULL;
...
+    }
+
+    ctxt = xmlXPathNewContext(xml);
+    if (ctxt == NULL) {
+        virReportOOMError();
+        goto cleanup;
return NULL;
...
+    }
+
+    ctxt->node = root;
+    def = virDomainBackupDefParse(ctxt, xmlopt, flags);
+ cleanup:
+    xmlXPathFreeContext(ctxt);
+    return def;
+}
+
+static int
+virDomainBackupDiskDefFormat(virBufferPtr buf,
+                             virDomainBackupDiskDefPtr disk,
+                             bool push, bool internal)
one arg per line

Should @internal make use of flags instead?
...
+{
+    int type = disk->store->type;
+    virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
+    virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+    int ret = -1;
+
+    if (!disk->name)
+        return 0;
+
+    virBufferEscapeString(buf, "<disk name='%s'", disk->name);
+    /* TODO: per-disk backup=off? */
... I assume this is the <backup> noted earlier.
...
+
+    virBufferAsprintf(buf, " type='%s'>\n", virStorageTypeToString(type));
+    virBufferAdjustIndent(buf, 2);
+
+    if (disk->store->format > 0)
+        virBufferEscapeString(buf, "<driver type='%s'/>\n",
+                              virStorageFileFormatTypeToString(disk->store->format));
+    /* TODO: should node names be part of storage file xml, rather
+     * than a one-off hack for qemu? */
Do we really want to store them in two places?  I think it's been hard
enough with just one.

Hazards of a design where the checkpoint and/or backup are not
subelements of the disk I suppose.
...
+    if (internal) {
+        virBufferEscapeString(buf, "<node detected='%s'",
+                              disk->store->detected ? "1" : "0");
+        virBufferEscapeString(buf, ">%s</node>\n", disk->store->nodeformat);
+    }
+
+    virBufferSetChildIndent(&childBuf, buf);
+    if (virDomainStorageSourceFormat(&attrBuf, &childBuf, disk->store, 0,
+                                     false) < 0)
+        goto cleanup;
+    if (virXMLFormatElement(buf, push ? "target" : "scratch",
+                            &attrBuf, &childBuf) < 0)
+        goto cleanup;
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</disk>\n");
+
+    ret = 0;
+
+ cleanup:
+    virBufferFreeAndReset(&attrBuf);
+    virBufferFreeAndReset(&childBuf);
+    return ret;
+}
+
+int
+virDomainBackupDefFormat(virBufferPtr buf, virDomainBackupDefPtr def,
+                         bool internal)
+{
+    size_t i;
+
+    virBufferAsprintf(buf, "<domainbackup mode='%s'",
+                      virDomainBackupTypeToString(def->type));
+    if (def->id)
+        virBufferAsprintf(buf, " id='%d'", def->id);> +    virBufferAddLit(buf, ">\n");
+    virBufferAdjustIndent(buf, 2);
+
+    virBufferEscapeString(buf, "<incremental>%s</incremental>\n",
+                          def->incremental);
+    if (def->server) {
+        virBufferAsprintf(buf, "<server transport='%s'",
+                          virStorageNetHostTransportTypeToString(def->server->transport));
+        virBufferEscapeString(buf, " name='%s'", def->server->name);
+        if (def->server->port)
+            virBufferAsprintf(buf, " port='%u'", def->server->port);
+        virBufferEscapeString(buf, " socket='%s'", def->server->socket);
+        virBufferAddLit(buf, "/>\n");
+    }
+
+    if (def->ndisks) {
+        virBufferAddLit(buf, "<disks>\n");
+        virBufferAdjustIndent(buf, 2);
+        for (i = 0; i < def->ndisks; i++) {
+            if (!def->disks[i].store)
+                continue;
+            if (virDomainBackupDiskDefFormat(buf, &def->disks[i],
+                                             def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH,
+                                             internal) < 0)
+                return -1;
+        }
+        virBufferAdjustIndent(buf, -2);
+        virBufferAddLit(buf, "</disks>\n");
+    }
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</domainbackup>\n");
+
+    return virBufferCheckError(buf);
+}
+
+
+static int
+virDomainBackupCompareDiskIndex(const void *a, const void *b)
+{
+    const virDomainBackupDiskDef *diska = a;
+    const virDomainBackupDiskDef *diskb = b;
+
+    /* Integer overflow shouldn't be a problem here.  */
+    return diska->idx - diskb->idx;
+}
+
+static int
+virDomainBackupDefAssignStore(virDomainBackupDiskDefPtr disk,
+                              virStorageSourcePtr src,
+                              const char *suffix)
+{
+    int ret = -1;
+
+    if (virStorageSourceIsEmpty(src)) {
+        if (disk->store) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("disk '%s' has no media"), disk->name);
+            goto cleanup;
+        }
+    } else if (src->readonly && disk->store) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("backup of readonly disk '%s' makes no sense"),
+                       disk->name);
+        goto cleanup;
+    } else if (!disk->store) {
+        if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_FILE) {
+            if (VIR_ALLOC(disk->store) < 0)
+                goto cleanup;
+            disk->store->type = VIR_STORAGE_TYPE_FILE;
+            if (virAsprintf(&disk->store->path, "%s.%s", src->path,
+                            suffix) < 0)
+                goto cleanup;
+            disk->store->detected = true;
+        } else {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("refusing to generate file name for disk '%s'"),
+                           disk->name);
+            goto cleanup;
+        }
+    }
does there need to be an else here ? e.g. can disk->store be set and
@src not assigned there?  The caller I assume knows to manage @src
afterwards.
...
+    ret = 0;
+ cleanup:
+    return ret;
Since there's no cleanup to be done, we could just return directly.
...
+}
+
+/* Align def->disks to domain.  Sort the list of def->disks,
+ * generating storage names using suffix as needed.  Convert paths to
+ * disk targets for uniformity.  Issue an error and return -1 if any
+ * def->disks[n]->name appears more than once or does not map to
+ * dom->disks. */
Similar locking concerns as previously - that is locking domain object.

This is really familiar code too - perhaps some code sharability is
possible.
...
+int
+virDomainBackupAlignDisks(virDomainBackupDefPtr def, virDomainDefPtr dom,
+                          const char *suffix)
+{
+    int ret = -1;
+    virBitmapPtr map = NULL;
+    size_t i;
+    int ndisks;
+    bool alloc_all = false;
VIR_AUTOPTR(virBitmap) map = NULL;

(could be useful other times too, but I just noticed it).
...
+
+    if (def->ndisks > dom->ndisks) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("too many disk backup requests for domain"));
+        goto cleanup;
+    }
+
+    /* Unlikely to have a guest without disks but technically possible.  */
+    if (!dom->ndisks) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("domain must have at least one disk to perform "
+                         "backups"));
+        goto cleanup;
+    }
+
+    if (!(map = virBitmapNew(dom->ndisks)))
+        goto cleanup;
+
+    /* Double check requested disks.  */
+    for (i = 0; i < def->ndisks; i++) {
+        virDomainBackupDiskDefPtr disk = &def->disks[i];
+        int idx = virDomainDiskIndexByName(dom, disk->name, false);
+
+        if (idx < 0) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("no disk named '%s'"), disk->name);
+            goto cleanup;
+        }
+
+        if (virBitmapIsBitSet(map, idx)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("disk '%s' specified twice"),
+                           disk->name);
+            goto cleanup;
+        }
+        ignore_value(virBitmapSetBit(map, idx));
+        disk->idx = idx;
+
+        if (STRNEQ(disk->name, dom->disks[idx]->dst)) {
Trying to picture how this happens... should this use STRNEQ_NULLABLE?
...
+            VIR_FREE(disk->name);
+            if (VIR_STRDUP(disk->name, dom->disks[idx]->dst) < 0)
+                goto cleanup;
+        }
+        if (disk->store && !disk->store->path) {
+            virStorageSourceClear(disk->store);
But no VIR_FREE(disk->source) how does that play with the following?
...
+            disk->store = NULL;
+        }
+        if (virDomainBackupDefAssignStore(disk, dom->disks[i]->src, suffix) < 0)
+            goto cleanup;
+    }
+
+    /* Provide fillers for all remaining disks, for easier iteration.  */
But is it "hooked up" with hotplug/hotunplug?

Beginning to think/wonder why backup/checkpoint isn't a child of storage
source.
...
+    if (!def->ndisks)
+        alloc_all = true;
+    ndisks = def->ndisks;
+    if (VIR_EXPAND_N(def->disks, def->ndisks,
+                     dom->ndisks - def->ndisks) < 0)
+        goto cleanup;
+
+    for (i = 0; i < dom->ndisks; i++) {
+        virDomainBackupDiskDefPtr disk;
+
+        if (virBitmapIsBitSet(map, i))
+            continue;
+        disk = &def->disks[ndisks++];
+        if (VIR_STRDUP(disk->name, dom->disks[i]->dst) < 0)
+            goto cleanup;
+        disk->idx = i;
+        if (alloc_all &&
+            virDomainBackupDefAssignStore(disk, dom->disks[i]->src, suffix) < 0)
+            goto cleanup;
+    }
+
+    qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
+          virDomainBackupCompareDiskIndex);
+
+    ret = 0;
+
+ cleanup:
+    virBitmapFree(map);
Using autofree stuff means the cleanup is unnecessary and we return
directly.


John
...
+    return ret;
+}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fbe7ba2d40..b9b7684494 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -69,6 +69,13 @@ virCapabilitiesSetNetPrefix;
# conf/checkpoint_conf.h
+virDomainBackupAlignDisks;
+virDomainBackupDefFormat;
+virDomainBackupDefFree;
+virDomainBackupDefParseNode;
+virDomainBackupDefParseString;
+virDomainBackupTypeFromString;
+virDomainBackupTypeToString;
 virDomainCheckpointAlignDisks;
 virDomainCheckpointAssignDef;
 virDomainCheckpointDefFormat;
@@ -88,7 +95,6 @@ virDomainCheckpointTypeToString;
 virDomainCheckpointUpdateRelations;
 virDomainListAllCheckpoints;
-
 # conf/cpu_conf.h
 virCPUCacheModeTypeFromString;
 virCPUCacheModeTypeToString;


    

Re: [libvirt] [PATCH v4 12/20] wip: backup: Parse and output backup XML

John Ferlan