On Mon, Jul 08, 2019 at 11:55:48 -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@redhat.com> ---
[...]
diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h new file mode 100644 index 0000000000..1714315a1f --- /dev/null +++ b/src/conf/backup_conf.h @@ -0,0 +1,94 @@
[...]
+/* 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; +};
[...]
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c new file mode 100644 index 0000000000..2bd94c1d73 --- /dev/null +++ b/src/conf/backup_conf.c @@ -0,0 +1,546 @@
[...]
+static void +virDomainBackupDiskDefClear(virDomainBackupDiskDefPtr disk) +{ + VIR_FREE(disk->name); + virStorageSourceClear(disk->store); + disk->store = NULL;
This leaks disk->store
+}
[...]
+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)
virStorageSource MUST be allocated using virStorageSourceNew since it's an virObject
+ 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 + + 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)) && + virDomainStorageSourceParse(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); + } + + 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'"),
Why is this qemuism validated in the main parser?
+ 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); + if (ret < 0) + virDomainBackupDiskDefClear(def); + return ret; +}
[...]
+static int +virDomainBackupDiskDefFormat(virBufferPtr buf, + virDomainBackupDiskDefPtr disk, + bool push, bool internal) +{ + 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? */ + + 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? */ + if (internal) { + virBufferEscapeString(buf, "<node detected='%s'", + disk->store->detected ? "1" : "0"); + virBufferEscapeString(buf, ">%s</node>\n", disk->store->nodeformat);
private data?
+ } + + if (virDomainDiskSourceFormat(buf, disk->store, push ? "target" : "scratch", + 0, false, 0, NULL) < 0) + goto cleanup; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</disk>\n"); + + ret = 0; + + cleanup: + virBufferFreeAndReset(&attrBuf); + virBufferFreeAndReset(&childBuf); + return ret; +} +
[...]
+static int +virDomainBackupDefAssignStore(virDomainBackupDiskDefPtr disk, + virStorageSourcePtr src, + const char *suffix) +{ + int ret = -1;
Please note in the comment that disk->store is used to switch behaviour when all_disks is true.
+ + 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)
virStorageSourceNew.
+ 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;
I already commented once that this should not be abused.
+ } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("refusing to generate file name for disk '%s'"), + disk->name); + goto cleanup; + } + } + ret = 0; + cleanup: + return ret; +} + +/* 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. */ +int +virDomainBackupAlignDisks(virDomainBackupDefPtr def, virDomainDefPtr dom, + const char *suffix) +{ + int ret = -1; + virBitmapPtr map = NULL; + size_t i; + int ndisks; + bool alloc_all = false; + + 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)) { + 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); + disk->store = NULL;
This will leak disk->store.
+ } + if (virDomainBackupDefAssignStore(disk, dom->disks[idx]->src, + suffix) < 0) + goto cleanup; + } + + /* Provide fillers for all remaining disks, for easier iteration. */ + 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;