[libvirt] [PATCH 0/5] virStorageBackendWipeLocal cleanups
The first patch fixes the return values of virStorageWipe on non-sparse local files in the case of a partial wipe or a fdatasync error. The rest reduces the number of parameters of virStorageBackendWipe{Extent,}Local. Ján Tomko (5): storage: fix return values of virStorageBackendWipeExtentLocal storage: move buffer allocation inside virStorageBackendWipeExtentLocal storage: drop 'Extent' from virStorageBackendWipeExtentLocal virStorageBackendWipeLocal: use unsigned long long instead of off_t virStorageBackendWipeLocal: remove bytes_wiped argument src/storage/storage_backend.c | 53 +++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 32 deletions(-) -- 2.4.6
Return -1: * on all failures of fdatasync. Instead of propagating -errno all the way up to the virStorageVolWipe API, which is documented to return 0 or -1. * after a partial wipe. If safewrite failed, we would re-use the non-negative return value of lseek (which should be 0 in this case, because that's the only offset we seek to). --- src/storage/storage_backend.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6915b8a..f62ebff 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2002,7 +2002,7 @@ virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, VIR_DEBUG("extent logical start: %ju len: %ju", (uintmax_t)extent_start, (uintmax_t)extent_length); - if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) { + if (lseek(fd, extent_start, SEEK_SET) < 0) { virReportSystemError(errno, _("Failed to seek to position %ju in volume " "with path '%s'"), @@ -2029,7 +2029,6 @@ virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, } if (fdatasync(fd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot sync data to volume with path '%s'"), vol->target.path); -- 2.4.6
We do not need to pass a zero-filled buffer as an argument, the function can allocate its own. --- src/storage/storage_backend.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f62ebff..120d654 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1991,17 +1991,20 @@ virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, int fd, off_t extent_start, off_t extent_length, - char *writebuf, size_t writebuf_length, size_t *bytes_wiped) { int ret = -1, written = 0; off_t remaining = 0; size_t write_size = 0; + char *writebuf = NULL; VIR_DEBUG("extent logical start: %ju len: %ju", (uintmax_t)extent_start, (uintmax_t)extent_length); + if (VIR_ALLOC_N(writebuf, writebuf_length) < 0) + goto cleanup; + if (lseek(fd, extent_start, SEEK_SET) < 0) { virReportSystemError(errno, _("Failed to seek to position %ju in volume " @@ -2041,6 +2044,7 @@ virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, ret = 0; cleanup: + VIR_FREE(writebuf); return ret; } @@ -2054,7 +2058,6 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret = -1, fd = -1; struct stat st; - char *writebuf = NULL; size_t bytes_wiped = 0; virCommandPtr cmd = NULL; @@ -2123,15 +2126,10 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd); } else { - - if (VIR_ALLOC_N(writebuf, st.st_blksize) < 0) - goto cleanup; - ret = virStorageBackendWipeExtentLocal(vol, fd, 0, vol->target.allocation, - writebuf, st.st_blksize, &bytes_wiped); } @@ -2139,7 +2137,6 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, cleanup: virCommandFree(cmd); - VIR_FREE(writebuf); VIR_FORCE_CLOSE(fd); return ret; } -- 2.4.6
The only caller always passes 0 for the extent start. Drop the 'extent_start' parameter, as well as the mention of extents from the function name. --- src/storage/storage_backend.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 120d654..d1276dd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1987,29 +1987,28 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, static int -virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, - int fd, - off_t extent_start, - off_t extent_length, - size_t writebuf_length, - size_t *bytes_wiped) +virStorageBackendWipeLocal(virStorageVolDefPtr vol, + int fd, + off_t extent_length, + size_t writebuf_length, + size_t *bytes_wiped) { int ret = -1, written = 0; off_t remaining = 0; size_t write_size = 0; char *writebuf = NULL; - VIR_DEBUG("extent logical start: %ju len: %ju", - (uintmax_t)extent_start, (uintmax_t)extent_length); + VIR_DEBUG("extent logical start: 0 len: %ju", + (uintmax_t)extent_length); if (VIR_ALLOC_N(writebuf, writebuf_length) < 0) goto cleanup; - if (lseek(fd, extent_start, SEEK_SET) < 0) { + if (lseek(fd, 0, SEEK_SET) < 0) { virReportSystemError(errno, - _("Failed to seek to position %ju in volume " + _("Failed to seek to the start in volume " "with path '%s'"), - (uintmax_t)extent_start, vol->target.path); + vol->target.path); goto cleanup; } @@ -2126,12 +2125,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd); } else { - ret = virStorageBackendWipeExtentLocal(vol, - fd, - 0, - vol->target.allocation, - st.st_blksize, - &bytes_wiped); + ret = virStorageBackendWipeLocal(vol, + fd, + vol->target.allocation, + st.st_blksize, + &bytes_wiped); } } -- 2.4.6
On 12/11/2015 11:36 AM, Ján Tomko wrote:
The only caller always passes 0 for the extent start. Drop the 'extent_start' parameter, as well as the mention of extents from the function name. --- src/storage/storage_backend.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)
I think 3 & 4 could be combined - since you're removing extent_start anyway as part of the same change... The extent_length/wipe_len is already an unsigned long long (from target.allocation) - so that's just part of fixing the function as far as I'm concerned. If you want to keep them separate I'm not going to complain either. John
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 120d654..d1276dd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1987,29 +1987,28 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol,
static int -virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, - int fd, - off_t extent_start, - off_t extent_length, - size_t writebuf_length, - size_t *bytes_wiped) +virStorageBackendWipeLocal(virStorageVolDefPtr vol, + int fd, + off_t extent_length, + size_t writebuf_length, + size_t *bytes_wiped) { int ret = -1, written = 0; off_t remaining = 0; size_t write_size = 0; char *writebuf = NULL;
- VIR_DEBUG("extent logical start: %ju len: %ju", - (uintmax_t)extent_start, (uintmax_t)extent_length); + VIR_DEBUG("extent logical start: 0 len: %ju", + (uintmax_t)extent_length);
if (VIR_ALLOC_N(writebuf, writebuf_length) < 0) goto cleanup;
- if (lseek(fd, extent_start, SEEK_SET) < 0) { + if (lseek(fd, 0, SEEK_SET) < 0) { virReportSystemError(errno, - _("Failed to seek to position %ju in volume " + _("Failed to seek to the start in volume " "with path '%s'"), - (uintmax_t)extent_start, vol->target.path); + vol->target.path); goto cleanup; }
@@ -2126,12 +2125,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd); } else { - ret = virStorageBackendWipeExtentLocal(vol, - fd, - 0, - vol->target.allocation, - st.st_blksize, - &bytes_wiped); + ret = virStorageBackendWipeLocal(vol, + fd, + vol->target.allocation, + st.st_blksize, + &bytes_wiped); } }
Change off_t extent_length to unsigned long long wipe_len, as well as the 'remain' variable. --- src/storage/storage_backend.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d1276dd..530177f 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1989,17 +1989,16 @@ virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, static int virStorageBackendWipeLocal(virStorageVolDefPtr vol, int fd, - off_t extent_length, + unsigned long long wipe_len, size_t writebuf_length, size_t *bytes_wiped) { int ret = -1, written = 0; - off_t remaining = 0; + unsigned long long remaining = 0; size_t write_size = 0; char *writebuf = NULL; - VIR_DEBUG("extent logical start: 0 len: %ju", - (uintmax_t)extent_length); + VIR_DEBUG("wiping start: 0 len: %llu", wipe_len); if (VIR_ALLOC_N(writebuf, writebuf_length) < 0) goto cleanup; @@ -2012,7 +2011,7 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, goto cleanup; } - remaining = extent_length; + remaining = wipe_len; while (remaining > 0) { write_size = (writebuf_length < remaining) ? writebuf_length : remaining; -- 2.4.6
It is not used by the caller. --- src/storage/storage_backend.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 530177f..03bc18c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1990,8 +1990,7 @@ static int virStorageBackendWipeLocal(virStorageVolDefPtr vol, int fd, unsigned long long wipe_len, - size_t writebuf_length, - size_t *bytes_wiped) + size_t writebuf_length) { int ret = -1, written = 0; unsigned long long remaining = 0; @@ -2025,7 +2024,6 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, goto cleanup; } - *bytes_wiped += written; remaining -= written; } @@ -2036,8 +2034,8 @@ virStorageBackendWipeLocal(virStorageVolDefPtr vol, goto cleanup; } - VIR_DEBUG("Wrote %zu bytes to volume with path '%s'", - *bytes_wiped, vol->target.path); + VIR_DEBUG("Wrote %llu bytes to volume with path '%s'", + wipe_len, vol->target.path); ret = 0; @@ -2056,7 +2054,6 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret = -1, fd = -1; struct stat st; - size_t bytes_wiped = 0; virCommandPtr cmd = NULL; virCheckFlags(0, -1); @@ -2127,8 +2124,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, ret = virStorageBackendWipeLocal(vol, fd, vol->target.allocation, - st.st_blksize, - &bytes_wiped); + st.st_blksize); } } -- 2.4.6
On 12/11/2015 11:36 AM, Ján Tomko wrote:
The first patch fixes the return values of virStorageWipe on non-sparse local files in the case of a partial wipe or a fdatasync error.
The rest reduces the number of parameters of virStorageBackendWipe{Extent,}Local.
Ján Tomko (5): storage: fix return values of virStorageBackendWipeExtentLocal storage: move buffer allocation inside virStorageBackendWipeExtentLocal storage: drop 'Extent' from virStorageBackendWipeExtentLocal virStorageBackendWipeLocal: use unsigned long long instead of off_t virStorageBackendWipeLocal: remove bytes_wiped argument
src/storage/storage_backend.c | 53 +++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 32 deletions(-)
Patch 1 note: (choose to add if you wish) The errno is printed anyway and thus unless someone overwrites your message should be passed back to the caller... I did make a comment in patch 3 - your call on how to handle. ACK series John
On Wed, Dec 16, 2015 at 03:24:31PM -0500, John Ferlan wrote:
On 12/11/2015 11:36 AM, Ján Tomko wrote:
The first patch fixes the return values of virStorageWipe on non-sparse local files in the case of a partial wipe or a fdatasync error.
The rest reduces the number of parameters of virStorageBackendWipe{Extent,}Local.
Ján Tomko (5): storage: fix return values of virStorageBackendWipeExtentLocal storage: move buffer allocation inside virStorageBackendWipeExtentLocal storage: drop 'Extent' from virStorageBackendWipeExtentLocal virStorageBackendWipeLocal: use unsigned long long instead of off_t virStorageBackendWipeLocal: remove bytes_wiped argument
src/storage/storage_backend.c | 53 +++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 32 deletions(-)
Patch 1 note: (choose to add if you wish)
The errno is printed anyway and thus unless someone overwrites your message should be passed back to the caller...
Most of our functions report a virError and the call is visible in the patch context.
I did make a comment in patch 3 - your call on how to handle.
ACK series
Thanks, I have squashed the two patches together and pushed the series. Jan
participants (2)
-
John Ferlan -
Ján Tomko