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@xxxxxxxxxx>
> ---
> 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;
>
