[libvirt] [PATCH 3/4] conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames

2016-02-11 Thread Peter Krempa
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 = >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, ) < 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(>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(>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, ) < 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(>src->path, "%s.%s", tmppath, def->name) < 0) {
+VIR_FREE(tmppath);
+return -1;
+}
+
+VIR_FREE(tmppath);
+}
+
+return 0;
 }


-- 
2.6.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH 3/4] conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames

2016-02-11 Thread Erik Skultety
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(>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(>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, ) < 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(>src->path, "%s.%s", tmppath, def->name) < 0) {
> +VIR_FREE(tmppath);
> +return -1;
> +}
> +
> +VIR_FREE(tmppath);
> +}
> +
> +return 0;
>  }
> 

Nicely done, ACK.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list