[libvirt] [PATCH 0/3] storage: Clean up some volume wiping code
Just a cleanup so that some functionality is more readable (about what's being done for ploop volumes when wiping them) and extensible in the future. Feel free to NACK if I'm the only one that finds this more readable. Martin Kletzander (3): storage: Move functions around storage: Use path instead of volume as an argument storage: Clean up volume wiping src/storage/storage_backend.c | 193 +++++++++++++++++++++++------------------- 1 file changed, 104 insertions(+), 89 deletions(-) -- 2.9.2
This is done in order to call them in next patches from each other and definitions would be missing otherwise. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_backend.c | 100 ++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6e99f3c31420..e0bba364f98e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2401,55 +2401,10 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, return ret; } -static int -virStorageBackendVolWipePloop(virStorageVolDefPtr vol) -{ - virCommandPtr cmd = NULL; - char *target_path = NULL; - char *disk_desc = NULL; - char *create_tool = NULL; - int ret = -1; +/* In here just for a clean patch series, will be removed in future patch */ +static int virStorageBackendVolWipePloop(virStorageVolDefPtr vol); - create_tool = virFindFileInPath("ploop"); - if (!create_tool) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unable to find ploop tools, please install them")); - return -1; - } - - if (virAsprintf(&target_path, "%s/root.hds", vol->target.path) < 0) - goto cleanup; - - if (virAsprintf(&disk_desc, "%s/DiskDescriptor.xml", vol->target.path) < 0) - goto cleanup; - - if (virFileRemove(disk_desc, 0, 0) < 0) { - virReportError(errno, _("Failed to delete DiskDescriptor.xml of volume '%s'"), - vol->target.path); - goto cleanup; - } - if (virFileRemove(target_path, 0, 0) < 0) { - virReportError(errno, _("failed to delete root.hds of volume '%s'"), - vol->target.path); - goto cleanup; - } - - cmd = virCommandNewArgList(create_tool, "init", "-s", NULL); - - virCommandAddArgFormat(cmd, "%lluM", VIR_DIV_UP(vol->target.capacity, - (1024 * 1024))); - virCommandAddArgList(cmd, "-t", "ext4", NULL); - virCommandAddArg(cmd, target_path); - ret = virCommandRun(cmd, NULL); - - cleanup: - VIR_FREE(disk_desc); - VIR_FREE(target_path); - VIR_FREE(create_tool); - virCommandFree(cmd); - return ret; -} int virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -2563,6 +2518,57 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } +static int +virStorageBackendVolWipePloop(virStorageVolDefPtr vol) +{ + virCommandPtr cmd = NULL; + char *target_path = NULL; + char *disk_desc = NULL; + char *create_tool = NULL; + + int ret = -1; + + create_tool = virFindFileInPath("ploop"); + if (!create_tool) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to find ploop tools, please install them")); + return -1; + } + + if (virAsprintf(&target_path, "%s/root.hds", vol->target.path) < 0) + goto cleanup; + + if (virAsprintf(&disk_desc, "%s/DiskDescriptor.xml", vol->target.path) < 0) + goto cleanup; + + if (virFileRemove(disk_desc, 0, 0) < 0) { + virReportError(errno, _("Failed to delete DiskDescriptor.xml of volume '%s'"), + vol->target.path); + goto cleanup; + } + if (virFileRemove(target_path, 0, 0) < 0) { + virReportError(errno, _("failed to delete root.hds of volume '%s'"), + vol->target.path); + goto cleanup; + } + + cmd = virCommandNewArgList(create_tool, "init", "-s", NULL); + + virCommandAddArgFormat(cmd, "%lluM", VIR_DIV_UP(vol->target.capacity, + (1024 * 1024))); + virCommandAddArgList(cmd, "-t", "ext4", NULL); + virCommandAddArg(cmd, target_path); + ret = virCommandRun(cmd, NULL); + + cleanup: + VIR_FREE(disk_desc); + VIR_FREE(target_path); + VIR_FREE(create_tool); + virCommandFree(cmd); + return ret; +} + + #ifdef GLUSTER_CLI int virStorageBackendFindGlusterPoolSources(const char *host, -- 2.9.2
Some functions use volume specification merely to use the target path from it. Let's change it to pass the path only so that it can be used for other files than just volumes. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_backend.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e0bba364f98e..b7c7af298f29 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2319,7 +2319,7 @@ virStorageBackendVolDownloadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, * appear as if it were zero-filled. */ static int -virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, +virStorageBackendVolZeroSparseFileLocal(const char *path, off_t size, int fd) { @@ -2327,7 +2327,7 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, virReportSystemError(errno, _("Failed to truncate volume with " "path '%s' to 0 bytes"), - vol->target.path); + path); return -1; } @@ -2335,7 +2335,7 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, virReportSystemError(errno, _("Failed to truncate volume with " "path '%s' to %ju bytes"), - vol->target.path, (uintmax_t)size); + path, (uintmax_t)size); return -1; } @@ -2344,7 +2344,7 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, static int -virStorageBackendWipeLocal(virStorageVolDefPtr vol, +virStorageBackendWipeLocal(const char *path, int fd, unsigned long long wipe_len, size_t writebuf_length) @@ -2363,7 +2363,7 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, virReportSystemError(errno, _("Failed to seek to the start in volume " "with path '%s'"), - vol->target.path); + path); goto cleanup; } @@ -2376,7 +2376,7 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, virReportSystemError(errno, _("Failed to write %zu bytes to " "storage volume with path '%s'"), - write_size, vol->target.path); + write_size, path); goto cleanup; } @@ -2387,12 +2387,11 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, if (fdatasync(fd) < 0) { virReportSystemError(errno, _("cannot sync data to volume with path '%s'"), - vol->target.path); + path); goto cleanup; } - VIR_DEBUG("Wrote %llu bytes to volume with path '%s'", - wipe_len, vol->target.path); + VIR_DEBUG("Wrote %llu bytes to volume with path '%s'", wipe_len, path); ret = 0; @@ -2496,9 +2495,9 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, ret = 0; } else { if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { - ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd); + ret = virStorageBackendVolZeroSparseFileLocal(path, st.st_size, fd); } else { - ret = virStorageBackendWipeLocal(vol, + ret = virStorageBackendWipeLocal(path, fd, vol->target.allocation, st.st_blksize); -- 2.9.2
Let's cleanly differentiate what wiping a volume does for ploop and other volumes so it's more readable what is done for each one instead of branching out multiple times in different parts of the same function. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_backend.c | 78 ++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b7c7af298f29..27a757edec73 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2401,47 +2401,28 @@ virStorageBackendWipeLocal(const char *path, } -/* In here just for a clean patch series, will be removed in future patch */ -static int virStorageBackendVolWipePloop(virStorageVolDefPtr vol); - - -int -virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol, - unsigned int algorithm, - unsigned int flags) +static int +virStorageBackendVolWipeLocalFile(const char *path, + unsigned int algorithm, + unsigned long long allocation) { int ret = -1, fd = -1; const char *alg_char = NULL; struct stat st; virCommandPtr cmd = NULL; - char *path = NULL; - char *target_path = vol->target.path; - virCheckFlags(0, -1); - - VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", - vol->target.path, algorithm); - - if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { - if (virAsprintf(&path, "%s/root.hds", vol->target.path) < 0) - goto cleanup; - target_path = path; - } - - fd = open(target_path, O_RDWR); + fd = open(path, O_RDWR); if (fd == -1) { virReportSystemError(errno, _("Failed to open storage volume with path '%s'"), - vol->target.path); + path); goto cleanup; } if (fstat(fd, &st) == -1) { virReportSystemError(errno, _("Failed to stat storage volume with path '%s'"), - vol->target.path); + path); goto cleanup; } @@ -2484,10 +2465,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } + VIR_DEBUG("Wiping file '%s' with algorithm '%s'", path, alg_char); + if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) { cmd = virCommandNew(SCRUB); - virCommandAddArgList(cmd, "-f", "-p", alg_char, - target_path, NULL); + virCommandAddArgList(cmd, "-f", "-p", alg_char, path, NULL); if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -2499,26 +2481,23 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } else { ret = virStorageBackendWipeLocal(path, fd, - vol->target.allocation, + allocation, st.st_blksize); } if (ret < 0) goto cleanup; } - if (vol->target.format == VIR_STORAGE_FILE_PLOOP) - ret = virStorageBackendVolWipePloop(vol); - cleanup: virCommandFree(cmd); - VIR_FREE(path); VIR_FORCE_CLOSE(fd); return ret; } static int -virStorageBackendVolWipePloop(virStorageVolDefPtr vol) +virStorageBackendVolWipePloop(virStorageVolDefPtr vol, + unsigned int algorithm) { virCommandPtr cmd = NULL; char *target_path = NULL; @@ -2540,6 +2519,11 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol) if (virAsprintf(&disk_desc, "%s/DiskDescriptor.xml", vol->target.path) < 0) goto cleanup; + if (virStorageBackendVolWipeLocalFile(target_path, + algorithm, + vol->target.allocation) < 0) + goto cleanup; + if (virFileRemove(disk_desc, 0, 0) < 0) { virReportError(errno, _("Failed to delete DiskDescriptor.xml of volume '%s'"), vol->target.path); @@ -2568,6 +2552,32 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol) } +int +virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags) +{ + int ret = -1; + + virCheckFlags(0, -1); + + VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", + vol->target.path, algorithm); + + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + ret = virStorageBackendVolWipePloop(vol, algorithm); + } else { + ret = virStorageBackendVolWipeLocalFile(vol->target.path, + algorithm, + vol->target.allocation); + } + + return ret; +} + + #ifdef GLUSTER_CLI int virStorageBackendFindGlusterPoolSources(const char *host, -- 2.9.2
On 08/01/2016 09:12 AM, Martin Kletzander wrote:
Let's cleanly differentiate what wiping a volume does for ploop and other volumes so it's more readable what is done for each one instead of branching out multiple times in different parts of the same function.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_backend.c | 78 ++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 34 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b7c7af298f29..27a757edec73 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2401,47 +2401,28 @@ virStorageBackendWipeLocal(const char *path, }
-/* In here just for a clean patch series, will be removed in future patch */ -static int virStorageBackendVolWipePloop(virStorageVolDefPtr vol); - - -int -virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol, - unsigned int algorithm, - unsigned int flags) +static int +virStorageBackendVolWipeLocalFile(const char *path, + unsigned int algorithm, + unsigned long long allocation) { int ret = -1, fd = -1; const char *alg_char = NULL; struct stat st; virCommandPtr cmd = NULL; - char *path = NULL; - char *target_path = vol->target.path;
- virCheckFlags(0, -1); - - VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", - vol->target.path, algorithm); - - if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { - if (virAsprintf(&path, "%s/root.hds", vol->target.path) < 0) - goto cleanup; - target_path = path; - } - - fd = open(target_path, O_RDWR); + fd = open(path, O_RDWR); if (fd == -1) { virReportSystemError(errno, _("Failed to open storage volume with path '%s'"), - vol->target.path); + path); goto cleanup; }
if (fstat(fd, &st) == -1) { virReportSystemError(errno, _("Failed to stat storage volume with path '%s'"), - vol->target.path); + path); goto cleanup; }
@@ -2484,10 +2465,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; }
+ VIR_DEBUG("Wiping file '%s' with algorithm '%s'", path, alg_char); + if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) { cmd = virCommandNew(SCRUB); - virCommandAddArgList(cmd, "-f", "-p", alg_char, - target_path, NULL); + virCommandAddArgList(cmd, "-f", "-p", alg_char, path, NULL);
if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -2499,26 +2481,23 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } else { ret = virStorageBackendWipeLocal(path, fd, - vol->target.allocation, + allocation, st.st_blksize); } if (ret < 0) goto cleanup; }
- if (vol->target.format == VIR_STORAGE_FILE_PLOOP) - ret = virStorageBackendVolWipePloop(vol); - cleanup: virCommandFree(cmd); - VIR_FREE(path); VIR_FORCE_CLOSE(fd); return ret; }
static int -virStorageBackendVolWipePloop(virStorageVolDefPtr vol) +virStorageBackendVolWipePloop(virStorageVolDefPtr vol, + unsigned int algorithm)
Ironically... this could take "path", "allocation", and "capacity" as parameters instead of "vol".... Your call on adjusting this one as well... If you do, you should adjust the odd vertical alignment for the virCommandAddArgFormat for VIR_DIV_UP(capacity,...) John
{ virCommandPtr cmd = NULL; char *target_path = NULL; @@ -2540,6 +2519,11 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol) if (virAsprintf(&disk_desc, "%s/DiskDescriptor.xml", vol->target.path) < 0) goto cleanup;
+ if (virStorageBackendVolWipeLocalFile(target_path, + algorithm, + vol->target.allocation) < 0) + goto cleanup; + if (virFileRemove(disk_desc, 0, 0) < 0) { virReportError(errno, _("Failed to delete DiskDescriptor.xml of volume '%s'"), vol->target.path); @@ -2568,6 +2552,32 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol) }
+int +virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags) +{ + int ret = -1; + + virCheckFlags(0, -1); + + VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", + vol->target.path, algorithm); + + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) { + ret = virStorageBackendVolWipePloop(vol, algorithm); + } else { + ret = virStorageBackendVolWipeLocalFile(vol->target.path, + algorithm, + vol->target.allocation); + } + + return ret; +} + + #ifdef GLUSTER_CLI int virStorageBackendFindGlusterPoolSources(const char *host,
On Mon, Aug 01, 2016 at 11:10:19AM -0400, John Ferlan wrote:
On 08/01/2016 09:12 AM, Martin Kletzander wrote:
Let's cleanly differentiate what wiping a volume does for ploop and other volumes so it's more readable what is done for each one instead of branching out multiple times in different parts of the same function.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_backend.c | 78 ++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 34 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b7c7af298f29..27a757edec73 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2499,26 +2481,23 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } else { ret = virStorageBackendWipeLocal(path, fd, - vol->target.allocation, + allocation, st.st_blksize); } if (ret < 0) goto cleanup; }
- if (vol->target.format == VIR_STORAGE_FILE_PLOOP) - ret = virStorageBackendVolWipePloop(vol); - cleanup: virCommandFree(cmd); - VIR_FREE(path); VIR_FORCE_CLOSE(fd); return ret; }
static int -virStorageBackendVolWipePloop(virStorageVolDefPtr vol) +virStorageBackendVolWipePloop(virStorageVolDefPtr vol, + unsigned int algorithm)
Ironically... this could take "path", "allocation", and "capacity" as parameters instead of "vol"....
I don't see the reason for that now as it won't be available from
Your call on adjusting this one as well... If you do, you should adjust the odd vertical alignment for the virCommandAddArgFormat for VIR_DIV_UP(capacity,...)
I adjusted that though. Let's have it clean. I'll push it after release. Martin
On 08/01/2016 09:11 AM, Martin Kletzander wrote:
Just a cleanup so that some functionality is more readable (about what's being done for ploop volumes when wiping them) and extensible in the future. Feel free to NACK if I'm the only one that finds this more readable.
Martin Kletzander (3): storage: Move functions around storage: Use path instead of volume as an argument storage: Clean up volume wiping
src/storage/storage_backend.c | 193 +++++++++++++++++++++++------------------- 1 file changed, 104 insertions(+), 89 deletions(-)
ACK series - your call on my point in 3/3 John
participants (2)
-
John Ferlan -
Martin Kletzander