Re: [PATCH v9 05/10] backup: Parse and output backup XML

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux