[PATCH 1/3] virStorageBackendCopyToFD: remove unused return variable
remove unused return variable, The errno will throw by virReportSystemError Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index cf1f33f177..6fc8597733 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -128,7 +128,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, bool reflink_copy) { int amtread = -1; - int ret = 0; size_t rbytes = READ_BLOCK_SIZE_DEFAULT; int wbytes = 0; int interval; @@ -138,11 +137,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, VIR_AUTOCLOSE inputfd = -1; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - ret = -errno; virReportSystemError(errno, _("could not open input path '%s'"), inputvol->target.path); - return ret; + return -1; } #ifdef __linux__ @@ -160,11 +158,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (reflink_copy) { if (reflinkCloneFile(fd, inputfd) < 0) { - ret = -errno; virReportSystemError(errno, _("failed to clone files from '%s'"), inputvol->target.path); - return ret; + return -1; } else { VIR_DEBUG("btrfs clone finished."); return 0; @@ -178,11 +175,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, rbytes = *total; if ((amtread = saferead(inputfd, buf, rbytes)) < 0) { - ret = -errno; virReportSystemError(errno, _("failed reading from file '%s'"), inputvol->target.path); - return ret; + return -1; } *total -= amtread; @@ -195,36 +191,32 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (want_sparse && memcmp(buf+offset, zerobuf, interval) == 0) { if (lseek(fd, interval, SEEK_CUR) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); - return ret; + return -1; } } else if (safewrite(fd, buf+offset, interval) < 0) { - ret = -errno; virReportSystemError(errno, _("failed writing to file '%s'"), vol->target.path); - return ret; + return -1; } } while ((amtleft -= interval) > 0); } if (virFileDataSync(fd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); - return ret; + return -1; } if (VIR_CLOSE(inputfd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot close file '%s'"), inputvol->target.path); - return ret; + return -1; } return 0; -- 2.25.3
remove unused return variable, The errno will throw by virReportSystemError Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 6fc8597733..c6d0f7a97c 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -307,7 +307,6 @@ createRawFile(int fd, virStorageVolDefPtr vol, bool reflink_copy) { bool need_alloc = true; - int ret = 0; unsigned long long pos = 0; /* If the new allocation is lower than the capacity of the original file, @@ -319,11 +318,10 @@ createRawFile(int fd, virStorageVolDefPtr vol, /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->target.capacity) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); - return ret; + return -1; } /* Avoid issues with older kernel's <linux/fs.h> namespace pollution. */ @@ -339,11 +337,10 @@ createRawFile(int fd, virStorageVolDefPtr vol, if (fallocate(fd, 0, 0, vol->target.allocation) == 0) { need_alloc = false; } else if (errno != ENOSYS && errno != EOPNOTSUPP) { - ret = -errno; virReportSystemError(errno, _("cannot allocate %llu bytes in file '%s'"), vol->target.allocation, vol->target.path); - return ret; + return -1; } } #endif @@ -353,9 +350,9 @@ createRawFile(int fd, virStorageVolDefPtr vol, /* allow zero blocks to be skipped if we've requested sparse * allocation (allocation < capacity) or we have already * been able to allocate the required space. */ - if ((ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, - !need_alloc, reflink_copy)) < 0) - return ret; + if (virStorageBackendCopyToFD(vol, inputvol, fd, &remain, + !need_alloc, reflink_copy) < 0) + return -1; /* If the new allocation is greater than the original capacity, * but fallocate failed, fill the rest with zeroes. @@ -365,21 +362,19 @@ createRawFile(int fd, virStorageVolDefPtr vol, if (need_alloc && (vol->target.allocation - pos > 0)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), vol->target.path); - return ret; + return -1; } } if (g_fsync(fd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); - return ret; + return -1; } - return ret; + return 0; } static int -- 2.25.3
refactor and remove unused created variable Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c6d0f7a97c..c02ece8253 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -384,11 +384,10 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - int ret = -1; + int ret = 0; int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; - bool created = false; VIR_AUTOCLOSE fd = -1; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -399,13 +398,13 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation is not supported for raw " "volumes")); - goto cleanup; + return -1; } if (virStorageSourceHasBacking(&vol->target)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for raw volumes")); - goto cleanup; + return -1; } if (flags & VIR_STORAGE_VOL_CREATE_REFLINK) @@ -415,7 +414,7 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, if (vol->target.encryption) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted volumes")); - goto cleanup; + return -1; } operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER; @@ -434,22 +433,23 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportSystemError(-fd, _("Failed to create file '%s'"), vol->target.path); - goto cleanup; + return -1; } - created = true; /* NB, COW flag can only be toggled when the file is zero-size, * so must go before the createRawFile call allocates payload */ if (vol->target.nocow && - virFileSetCOW(vol->target.path, VIR_TRISTATE_BOOL_NO) < 0) + virFileSetCOW(vol->target.path, VIR_TRISTATE_BOOL_NO) < 0) { + ret = -1; goto cleanup; + } - if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) + if (createRawFile(fd, vol, inputvol, reflink_copy) < 0) /* createRawFile already reported the exact error. */ ret = -1; cleanup: - if (ret < 0 && created) + if (ret < 0) ignore_value(virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid)); -- 2.25.3
On 12/29/20 12:29 PM, Yi Li wrote:
refactor and remove unused created variable
Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c6d0f7a97c..c02ece8253 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -384,11 +384,10 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - int ret = -1; + int ret = 0;
No, please don't initialize this to zero. The pattern we use (and are used to) is: int ret = -1; if (something) goto cleanup; if (something else) goto cleanup; ret = 0; cleanup: if (ret < 0) cleanupWhatsNeeded(); return ret; Alternatively, we can rename 'cleanup' to 'error' and do the following (without even having to use @ret variable): if (something) goto error; if (something else) goto error; return 0; error: cleanupWhatsNeeded(); return -1; Michal
refactor storageBackendCreateRaw and remove some unused variable. Yi Li (3): createRawFile: remove unused return variable virStorageBackendCopyToFD: remove unused return variable storageBackendCreateRaw: remove unused created src/storage/storage_util.c | 66 +++++++++++++++----------------------- 1 file changed, 25 insertions(+), 41 deletions(-) -- 2.25.3
remove unused return variable, The errno will throw by virReportSystemError Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index cf1f33f177..c13abf03af 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -315,7 +315,6 @@ createRawFile(int fd, virStorageVolDefPtr vol, bool reflink_copy) { bool need_alloc = true; - int ret = 0; unsigned long long pos = 0; /* If the new allocation is lower than the capacity of the original file, @@ -327,11 +326,10 @@ createRawFile(int fd, virStorageVolDefPtr vol, /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->target.capacity) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); - return ret; + return -1; } /* Avoid issues with older kernel's <linux/fs.h> namespace pollution. */ @@ -347,11 +345,10 @@ createRawFile(int fd, virStorageVolDefPtr vol, if (fallocate(fd, 0, 0, vol->target.allocation) == 0) { need_alloc = false; } else if (errno != ENOSYS && errno != EOPNOTSUPP) { - ret = -errno; virReportSystemError(errno, _("cannot allocate %llu bytes in file '%s'"), vol->target.allocation, vol->target.path); - return ret; + return -1; } } #endif @@ -361,9 +358,9 @@ createRawFile(int fd, virStorageVolDefPtr vol, /* allow zero blocks to be skipped if we've requested sparse * allocation (allocation < capacity) or we have already * been able to allocate the required space. */ - if ((ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, - !need_alloc, reflink_copy)) < 0) - return ret; + if (virStorageBackendCopyToFD(vol, inputvol, fd, &remain, + !need_alloc, reflink_copy) < 0) + return -1; /* If the new allocation is greater than the original capacity, * but fallocate failed, fill the rest with zeroes. @@ -373,21 +370,19 @@ createRawFile(int fd, virStorageVolDefPtr vol, if (need_alloc && (vol->target.allocation - pos > 0)) { if (safezero(fd, pos, vol->target.allocation - pos) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), vol->target.path); - return ret; + return -1; } } if (g_fsync(fd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); - return ret; + return -1; } - return ret; + return 0; } static int -- 2.25.3
remove unused return variable, The errno will throw by virReportSystemError Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c13abf03af..c6d0f7a97c 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -128,7 +128,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, bool reflink_copy) { int amtread = -1; - int ret = 0; size_t rbytes = READ_BLOCK_SIZE_DEFAULT; int wbytes = 0; int interval; @@ -138,11 +137,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, VIR_AUTOCLOSE inputfd = -1; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - ret = -errno; virReportSystemError(errno, _("could not open input path '%s'"), inputvol->target.path); - return ret; + return -1; } #ifdef __linux__ @@ -160,11 +158,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (reflink_copy) { if (reflinkCloneFile(fd, inputfd) < 0) { - ret = -errno; virReportSystemError(errno, _("failed to clone files from '%s'"), inputvol->target.path); - return ret; + return -1; } else { VIR_DEBUG("btrfs clone finished."); return 0; @@ -178,11 +175,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, rbytes = *total; if ((amtread = saferead(inputfd, buf, rbytes)) < 0) { - ret = -errno; virReportSystemError(errno, _("failed reading from file '%s'"), inputvol->target.path); - return ret; + return -1; } *total -= amtread; @@ -195,36 +191,32 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (want_sparse && memcmp(buf+offset, zerobuf, interval) == 0) { if (lseek(fd, interval, SEEK_CUR) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); - return ret; + return -1; } } else if (safewrite(fd, buf+offset, interval) < 0) { - ret = -errno; virReportSystemError(errno, _("failed writing to file '%s'"), vol->target.path); - return ret; + return -1; } } while ((amtleft -= interval) > 0); } if (virFileDataSync(fd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); - return ret; + return -1; } if (VIR_CLOSE(inputfd) < 0) { - ret = -errno; virReportSystemError(errno, _("cannot close file '%s'"), inputvol->target.path); - return ret; + return -1; } return 0; -- 2.25.3
refactor and remove unused created variable. Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c6d0f7a97c..cc8189c3e0 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -384,11 +384,9 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - int ret = -1; int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; - bool created = false; VIR_AUTOCLOSE fd = -1; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -399,13 +397,13 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation is not supported for raw " "volumes")); - goto cleanup; + return -1; } if (virStorageSourceHasBacking(&vol->target)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for raw volumes")); - goto cleanup; + return -1; } if (flags & VIR_STORAGE_VOL_CREATE_REFLINK) @@ -415,7 +413,7 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, if (vol->target.encryption) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted volumes")); - goto cleanup; + return -1; } operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER; @@ -434,26 +432,25 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportSystemError(-fd, _("Failed to create file '%s'"), vol->target.path); - goto cleanup; + return -1; } - created = true; /* NB, COW flag can only be toggled when the file is zero-size, * so must go before the createRawFile call allocates payload */ if (vol->target.nocow && virFileSetCOW(vol->target.path, VIR_TRISTATE_BOOL_NO) < 0) - goto cleanup; + goto error; - if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) + if (createRawFile(fd, vol, inputvol, reflink_copy) < 0) /* createRawFile already reported the exact error. */ - ret = -1; + goto error; + return 0; - cleanup: - if (ret < 0 && created) + error: ignore_value(virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid)); - return ret; + return -1; } -- 2.25.3
On 1/5/21 3:43 PM, Yi Li wrote:
refactor and remove unused created variable.
Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c6d0f7a97c..cc8189c3e0 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -384,11 +384,9 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - int ret = -1; int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; - bool created = false; VIR_AUTOCLOSE fd = -1;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -399,13 +397,13 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation is not supported for raw " "volumes")); - goto cleanup; + return -1; }
if (virStorageSourceHasBacking(&vol->target)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("backing storage not supported for raw volumes")); - goto cleanup; + return -1; }
if (flags & VIR_STORAGE_VOL_CREATE_REFLINK) @@ -415,7 +413,7 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, if (vol->target.encryption) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted volumes")); - goto cleanup; + return -1; }
operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER; @@ -434,26 +432,25 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, virReportSystemError(-fd, _("Failed to create file '%s'"), vol->target.path); - goto cleanup; + return -1; } - created = true;
/* NB, COW flag can only be toggled when the file is zero-size, * so must go before the createRawFile call allocates payload */ if (vol->target.nocow && virFileSetCOW(vol->target.path, VIR_TRISTATE_BOOL_NO) < 0) - goto cleanup; + goto error;
- if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) + if (createRawFile(fd, vol, inputvol, reflink_copy) < 0) /* createRawFile already reported the exact error. */ - ret = -1; + goto error;
Since you're touching these lines, they deserve to be wrapped in curly braces - any body with two or more lines (including comments) must be, according to our coding style.
+ return 0;
- cleanup: - if (ret < 0 && created) + error: ignore_value(virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid));
This should be re-aligned. And the ignore_value() is needless - virFileRemove() is not declared with G_GNUC_WARN_UNUSED_RESULT and never was.
- return ret; + return -1; }
I'm fixing small nits I've found locally before pushing. Michal
On 1/5/21 3:43 PM, Yi Li wrote:
refactor storageBackendCreateRaw and remove some unused variable.
Yi Li (3): createRawFile: remove unused return variable virStorageBackendCopyToFD: remove unused return variable storageBackendCreateRaw: remove unused created
src/storage/storage_util.c | 66 +++++++++++++++----------------------- 1 file changed, 25 insertions(+), 41 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
On 12/29/20 12:29 PM, Yi Li wrote:
remove unused return variable, The errno will throw by virReportSystemError
Signed-off-by: Yi Li <yili@winhong.com> --- src/storage/storage_util.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
Patches look good, but I'd reorder them a bit. 2/3 should go first, then this on and 3/3 the last. Also, can you please send v2 with cover letter? It's easy as 'git format-patch -v2 --cover-letter ...' Michal
participants (2)
-
Michal Privoznik -
Yi Li