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@xxxxxxxxxx>
> ---
[...]
> 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;
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
