[libvirt] [PATCH v8 11/21] backup: Parse and output backup XML

Peter Krempa pkrempa at redhat.com
Thu Apr 25 13:07:24 UTC 2019


On Wed, Apr 17, 2019 at 09:09:11 -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 at redhat.com>
> ---
>  src/conf/backup_conf.h   |  97 +++++++
>  src/conf/virconftypes.h  |   3 +
>  src/conf/Makefile.inc.am |   2 +
>  src/conf/backup_conf.c   | 538 +++++++++++++++++++++++++++++++++++++++
>  src/libvirt_private.syms |   8 +-
>  5 files changed, 647 insertions(+), 1 deletion(-)
>  create mode 100644 src/conf/backup_conf.h
>  create mode 100644 src/conf/backup_conf.c

Just some notes since this has a lot of commented out code and thus
probably is incomplete.

As I've mentioned for the checkpoint XML the parser should be separate
from the validation. Also the parser should always validate the XML
schema prior to parsing it since this will be accessed via new APIs.

I've also pointed out some problems I've seen while reading through.

> diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h
> new file mode 100644
> index 0000000000..74edb601f8
> --- /dev/null
> +++ b/src/conf/backup_conf.h
> @@ -0,0 +1,97 @@
> +/*
> + * backup_conf.h: domain backup XML processing
> + *                 (based on domain_conf.h)
> + *
> + * Copyright (C) 2006-2019 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef LIBVIRT_BACKUP_CONF_H
> +# define LIBVIRT_BACKUP_CONF_H
> +
> +# include "internal.h"
> +# include "domain_conf.h"
> +# include "moment_conf.h"
> +# include "virenum.h"
> +
> +/* 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 */
> +    VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE, /* Push job finished */

The state labels are quite qemuistic. Should we choose something more
generic here?

> +} 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;
> +};
> +
> +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);

These should probably follow the new coding guideline for header
styling.

> +
> +#endif /* LIBVIRT_BACKUP_CONF_H */

[...]

> diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
> new file mode 100644
> index 0000000000..723d6bad4b
> --- /dev/null
> +++ b/src/conf/backup_conf.c
> @@ -0,0 +1,538 @@
> +/*
> + * backup_conf.c: domain backup XML processing
> + *
> + * Copyright (C) 2006-2019 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include "internal.h"
> +#include "virbitmap.h"
> +#include "virbuffer.h"
> +#include "datatypes.h"
> +#include "domain_conf.h"
> +#include "virlog.h"
> +#include "viralloc.h"
> +#include "backup_conf.h"
> +#include "virstoragefile.h"
> +#include "viruuid.h"
> +#include "virfile.h"
> +#include "virerror.h"
> +#include "virxml.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_DOMAIN
> +
> +VIR_LOG_INIT("conf.backup_conf");
> +
> +VIR_ENUM_IMPL(virDomainBackup,
> +              VIR_DOMAIN_BACKUP_TYPE_LAST,
> +              "default", "push", "pull");
> +
> +static void
> +virDomainBackupDiskDefClear(virDomainBackupDiskDefPtr disk)
> +{
> +    VIR_FREE(disk->name);
> +    virStorageSourceClear(disk->store);

This does not free disk->store itself.

> +    disk->store = NULL;
> +}
> +
> +void
> +virDomainBackupDefFree(virDomainBackupDefPtr def)
> +{
> +    size_t i;
> +
> +    if (!def)
> +        return;
> +
> +    VIR_FREE(def->incremental);
> +    virStorageNetHostDefFree(1, def->server);
> +    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;
> +
> +    ctxt->node = node;
> +
> +    if (VIR_ALLOC(def->store) < 0)

This needs to use virStorageSourceNew now since it's an object.

> +        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'"),
> +                           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 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;
> +
> +    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");
> +        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>"));
> +            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);
> +
> +    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? */

No. They are qemuisms.

> +    if (internal) {
> +        virBufferEscapeString(buf, "<node detected='%s'",
> +                              disk->store->detected ? "1" : "0");

What is this used for? As I've said above the intended use of the field
was different.

> +        virBufferEscapeString(buf, ">%s</node>\n", disk->store->nodeformat);

Looks like you need private data formatters/parsers.

> +    }
> +
> +    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;
> +}
> +
> +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)

virStorageSourceNew

> +                goto cleanup;
> +            disk->store->type = VIR_STORAGE_TYPE_FILE;
> +            if (virAsprintf(&disk->store->path, "%s.%s", src->path,
> +                            suffix) < 0)

I'm contemplating whether it actually makes sense to be as kind to the
user to allow ommitting this. We could make the user pass in the path
unconditionally.

> +                goto cleanup;
> +            disk->store->detected = true;

this field is meant to be used as 'detected by the backing chain
detector'.

> +        } else {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("refusing to generate file name for disk '%s'"),
> +                           disk->name);
> +            goto cleanup;
> +        }
> +    }
> +    ret = 0;
> + cleanup:

Nothing to clean up.

> +    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;
> +        }
> +        if (virDomainBackupDefAssignStore(disk, dom->disks[i]->src, suffix) < 0)
> +            goto cleanup;
> +    }
> +
> +    /* Provide fillers for all remaining disks, for easier iteration.  */
> +    if (!def->ndisks)

Please use an explicit comparison with zero. Also if we don't provide
the way to generate names this won't be necessary.

> +        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);
> +    return ret;
> +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190425/ff0792d1/attachment-0001.sig>


More information about the libvir-list mailing list