[libvirt] [PATCH 0/4] conf: snapshot: refactor and fix autogenerated duplicate snapshot targets
Peter Krempa (4): conf: snapshot: Rename disksorter to virDomainSnapshotCompareDiskIndex snapshot: conf: Extract code to generate default external file names conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames conf: snapshot: Avoid autogenerating duplicate snapshot names src/conf/snapshot_conf.c | 142 ++++++++++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 56 deletions(-) -- 2.6.2
Stick to the naming pattern. --- src/conf/snapshot_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1eda7c2..65a78d6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -430,7 +430,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, } static int -disksorter(const void *a, const void *b) +virDomainSnapshotCompareDiskIndex(const void *a, const void *b) { const virDomainSnapshotDiskDef *diska = a; const virDomainSnapshotDiskDef *diskb = b; @@ -562,7 +562,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->snapshot = default_snapshot; } - qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), + virDomainSnapshotCompareDiskIndex); /* Generate any default external file names, but only if the * backing file is a regular file. */ -- 2.6.2
On 11/02/16 10:20, Peter Krempa wrote:
Stick to the naming pattern. --- src/conf/snapshot_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1eda7c2..65a78d6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -430,7 +430,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, }
static int -disksorter(const void *a, const void *b) +virDomainSnapshotCompareDiskIndex(const void *a, const void *b) { const virDomainSnapshotDiskDef *diska = a; const virDomainSnapshotDiskDef *diskb = b; @@ -562,7 +562,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->snapshot = default_snapshot; }
- qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), + virDomainSnapshotCompareDiskIndex);
/* Generate any default external file names, but only if the * backing file is a regular file. */
ACK Erik
--- src/conf/snapshot_conf.c | 131 ++++++++++++++++++++++++++++------------------- 1 file changed, 77 insertions(+), 54 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 65a78d6..da6fec5 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -429,6 +429,80 @@ virDomainSnapshotDefParseString(const char *xmlStr, return ret; } + +/** + * virDomainSnapshotDefAssignExternalNames: + * @def: snapshot def object + * + * Generate default external file names for snapshot targets. Returns 0 on + * success, -1 on error. + */ +static int +virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def) +{ + size_t i; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && + !disk->src->path) { + const char *original = virDomainDiskGetSource(def->dom->disks[i]); + const char *tmp; + struct stat sb; + + if (disk->src->type != VIR_STORAGE_TYPE_FILE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' on a '%s' device"), + disk->name, + virStorageTypeToString(disk->src->type)); + goto cleanup; + } + + if (!original) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' without source"), + disk->name); + goto cleanup; + } + if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("source for disk '%s' is not a regular " + "file; refusing to generate external " + "snapshot name"), + disk->name); + goto cleanup; + } + + tmp = strrchr(original, '.'); + if (!tmp || strchr(tmp, '/')) { + if (virAsprintf(&disk->src->path, "%s.%s", original, + def->name) < 0) + goto cleanup; + } else { + if ((tmp - original) > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("integer overflow")); + goto cleanup; + } + if (virAsprintf(&disk->src->path, "%.*s.%s", + (int) (tmp - original), original, + def->name) < 0) + goto cleanup; + } + } + } + + ret = 0; + + cleanup: + return ret; +} + + static int virDomainSnapshotCompareDiskIndex(const void *a, const void *b) { @@ -565,60 +639,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), virDomainSnapshotCompareDiskIndex); - /* Generate any default external file names, but only if the - * backing file is a regular file. */ - for (i = 0; i < def->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - - if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && - !disk->src->path) { - const char *original = virDomainDiskGetSource(def->dom->disks[i]); - const char *tmp; - struct stat sb; - - if (disk->src->type != VIR_STORAGE_TYPE_FILE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot generate external snapshot name " - "for disk '%s' on a '%s' device"), - disk->name, - virStorageTypeToString(disk->src->type)); - goto cleanup; - } - - if (!original) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot generate external snapshot name " - "for disk '%s' without source"), - disk->name); - goto cleanup; - } - if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("source for disk '%s' is not a regular " - "file; refusing to generate external " - "snapshot name"), - disk->name); - goto cleanup; - } - - tmp = strrchr(original, '.'); - if (!tmp || strchr(tmp, '/')) { - if (virAsprintf(&disk->src->path, "%s.%s", original, - def->name) < 0) - goto cleanup; - } else { - if ((tmp - original) > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("integer overflow")); - goto cleanup; - } - if (virAsprintf(&disk->src->path, "%.*s.%s", - (int) (tmp - original), original, - def->name) < 0) - goto cleanup; - } - } - } + /* Generate default external file names for external snapshot locations */ + if (virDomainSnapshotDefAssignExternalNames(def) < 0) + goto cleanup; ret = 0; -- 2.6.2
On 11/02/16 10:20, Peter Krempa wrote:
--- src/conf/snapshot_conf.c | 131 ++++++++++++++++++++++++++++------------------- 1 file changed, 77 insertions(+), 54 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 65a78d6..da6fec5 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -429,6 +429,80 @@ virDomainSnapshotDefParseString(const char *xmlStr, return ret; } ...
ACK Erik
Get rid of one indentation level by negating condition and remove ugly pointer arithmetic at the cost of one extra allocation. --- src/conf/snapshot_conf.c | 94 +++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 50 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index da6fec5..f8a1aed 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -440,66 +440,60 @@ virDomainSnapshotDefParseString(const char *xmlStr, static int virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def) { + const char *origpath; + char *tmppath; + char *tmp; + struct stat sb; size_t i; - int ret = -1; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && - !disk->src->path) { - const char *original = virDomainDiskGetSource(def->dom->disks[i]); - const char *tmp; - struct stat sb; - - if (disk->src->type != VIR_STORAGE_TYPE_FILE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot generate external snapshot name " - "for disk '%s' on a '%s' device"), - disk->name, - virStorageTypeToString(disk->src->type)); - goto cleanup; - } + if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + disk->src->path) + continue; - if (!original) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot generate external snapshot name " - "for disk '%s' without source"), - disk->name); - goto cleanup; - } - if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("source for disk '%s' is not a regular " - "file; refusing to generate external " - "snapshot name"), - disk->name); - goto cleanup; - } + if (disk->src->type != VIR_STORAGE_TYPE_FILE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' on a '%s' device"), + disk->name, virStorageTypeToString(disk->src->type)); + return -1; + } - tmp = strrchr(original, '.'); - if (!tmp || strchr(tmp, '/')) { - if (virAsprintf(&disk->src->path, "%s.%s", original, - def->name) < 0) - goto cleanup; - } else { - if ((tmp - original) > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("integer overflow")); - goto cleanup; - } - if (virAsprintf(&disk->src->path, "%.*s.%s", - (int) (tmp - original), original, - def->name) < 0) - goto cleanup; - } + if (!(origpath = virDomainDiskGetSource(def->dom->disks[i]))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' without source"), + disk->name); + return -1; } - } - ret = 0; + if (stat(origpath, &sb) < 0 || !S_ISREG(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("source for disk '%s' is not a regular " + "file; refusing to generate external " + "snapshot name"), + disk->name); + return -1; + } - cleanup: - return ret; + if (VIR_STRDUP(tmppath, origpath) < 0) + return -1; + + /* drop suffix of the file name */ + if ((tmp = strrchr(tmppath, '.')) && !strchr(tmp, '/')) + *tmp = '\0'; + + if (virAsprintf(&disk->src->path, "%s.%s", tmppath, def->name) < 0) { + VIR_FREE(tmppath); + return -1; + } + + VIR_FREE(tmppath); + } + + return 0; } -- 2.6.2
On 11/02/16 10:20, Peter Krempa wrote:
Get rid of one indentation level by negating condition and remove ugly pointer arithmetic at the cost of one extra allocation. --- src/conf/snapshot_conf.c | 94 +++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 50 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index da6fec5..f8a1aed 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -440,66 +440,60 @@ virDomainSnapshotDefParseString(const char *xmlStr, ... ... - tmp = strrchr(original, '.'); - if (!tmp || strchr(tmp, '/')) { - if (virAsprintf(&disk->src->path, "%s.%s", original, - def->name) < 0) - goto cleanup; - } else { - if ((tmp - original) > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("integer overflow")); - goto cleanup; - } - if (virAsprintf(&disk->src->path, "%.*s.%s", - (int) (tmp - original), original, - def->name) < 0) - goto cleanup; - }
This was black magic indeed.
+ if (!(origpath = virDomainDiskGetSource(def->dom->disks[i]))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' without source"), + disk->name); + return -1; } - }
- ret = 0; + if (stat(origpath, &sb) < 0 || !S_ISREG(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("source for disk '%s' is not a regular " + "file; refusing to generate external " + "snapshot name"), + disk->name); + return -1; + }
- cleanup: - return ret; + if (VIR_STRDUP(tmppath, origpath) < 0) + return -1; + + /* drop suffix of the file name */ + if ((tmp = strrchr(tmppath, '.')) && !strchr(tmp, '/')) + *tmp = '\0'; + + if (virAsprintf(&disk->src->path, "%s.%s", tmppath, def->name) < 0) { + VIR_FREE(tmppath); + return -1; + } + + VIR_FREE(tmppath); + } + + return 0; }
Nicely done, ACK. Erik
The snapshot name generator truncates the original file name after a '.' and replaces the suffix with the snapshot name. If two disks source images differ only in the suffix portion, the generated name will be duplicate. Since this is a corner case just error out stating that a duplicate name was generated. The user can work around this situation by providing the file names explicitly. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085 --- src/conf/snapshot_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f8a1aed..58646bd 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -445,6 +445,7 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def) char *tmp; struct stat sb; size_t i; + size_t j; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; @@ -491,6 +492,17 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def) } VIR_FREE(tmppath); + + /* verify that we didn't generate a duplicate name */ + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(disk->src->path, def->disks[j].src->path)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name for " + "disk '%s': collision with disk '%s'"), + disk->name, def->disks[j].name); + return -1; + } + } } return 0; -- 2.6.2
On 11/02/16 10:20, Peter Krempa wrote:
The snapshot name generator truncates the original file name after a '.' and replaces the suffix with the snapshot name. If two disks source images differ only in the suffix portion, the generated name will be duplicate.
Since this is a corner case just error out stating that a duplicate name was generated. The user can work around this situation by providing the file names explicitly.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085 --- src/conf/snapshot_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
... ACK Erik
On Thu, Feb 11, 2016 at 15:24:42 +0100, Erik Skultety wrote:
On 11/02/16 10:20, Peter Krempa wrote:
The snapshot name generator truncates the original file name after a '.' and replaces the suffix with the snapshot name. If two disks source images differ only in the suffix portion, the generated name will be duplicate.
Since this is a corner case just error out stating that a duplicate name was generated. The user can work around this situation by providing the file names explicitly.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283085 --- src/conf/snapshot_conf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
...
ACK
Thanks, I've pushed the series. Peter
participants (2)
-
Erik Skultety -
Peter Krempa