[v2,1/3] btrfs-progs: fi defrag: clean up duplicate code if find errors
diff mbox

Message ID 20171128091450.21789-1-suy.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Su Yue Nov. 28, 2017, 9:14 a.m. UTC
In function cmd_filesystem_defrag(), lines of code for error handling
are duplicate and hard to expand in further.

Create a jump label for errors.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:	Record -errno to return value if open() and fstat() failed.
	Move change "do no exit if defrag range ioctl is unsupported"
	to another patch.
	Suggested by David.
---
 cmds-filesystem.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

Comments

David Sterba Nov. 28, 2017, 4:07 p.m. UTC | #1
On Tue, Nov 28, 2017 at 05:14:48PM +0800, Su Yue wrote:
> In function cmd_filesystem_defrag(), lines of code for error handling
> are duplicate and hard to expand in further.
> 
> Create a jump label for errors.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Nov. 30, 2017, 6:47 a.m. UTC | #2
On 2017年11月28日 17:14, Su Yue wrote:
> In function cmd_filesystem_defrag(), lines of code for error handling
> are duplicate and hard to expand in further.
> 
> Create a jump label for errors.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
> Changelog:
> v2:	Record -errno to return value if open() and fstat() failed.
> 	Move change "do no exit if defrag range ioctl is unsupported"
> 	to another patch.
> 	Suggested by David.
> ---
>  cmds-filesystem.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 7728430f16a1..17d399d58adf 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -1029,23 +1029,22 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		if (fd < 0) {
>  			error("cannot open %s: %s", argv[i],
>  					strerror(errno));
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -errno;
> +			goto next;
>  		}
> -		if (fstat(fd, &st)) {
> +
> +		ret = fstat(fd, &st);
> +		if (ret) {
>  			error("failed to stat %s: %s",
>  					argv[i], strerror(errno));
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -errno;
> +			goto next;
>  		}
>  		if (!(S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
>  			error("%s is not a directory or a regular file",
>  					argv[i]);
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -EINVAL;
> +			goto next;

Here, unlike the ftw callback, which ignore non-regular file, here we
count them as error.

So if there is some especial file in dir (use "example_dir" as an
example), then
# btrfs fi defrag -r example_dir
will return no error.

While
# btrfs fi defrag -r example_dir/*
Will return error.

IIRC such inconsistent behavior is a bigger problem, and reusing the
defrag_callback() will not only makes the behavior consistent, but also
simplify the loop.

Thanks,
Qu


>  		}
>  		if (recursive && S_ISDIR(st.st_mode)) {
>  			ret = nftw(argv[i], defrag_callback, 10,
> @@ -1060,20 +1059,25 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  			ret = ioctl(fd, BTRFS_IOC_DEFRAG_RANGE,
>  					&defrag_global_range);
>  			defrag_err = errno;
> -		}
> -		close_file_or_dir(fd, dirstream);
> -		if (ret && defrag_err == ENOTTY) {
> -			error(
> +			if (ret && defrag_err == ENOTTY) {
> +				error(
>  "defrag range ioctl not supported in this kernel version, 2.6.33 and newer is required");
> -			defrag_global_errors++;
> -			break;
> +				defrag_global_errors++;
> +				close_file_or_dir(fd, dirstream);
> +				break;
> +			}
> +			if (ret) {
> +				error("defrag failed on %s: %s", argv[i],
> +				      strerror(defrag_err));
> +				goto next;
> +			}
>  		}
> -		if (ret) {
> -			error("defrag failed on %s: %s", argv[i],
> -					strerror(defrag_err));
> +next:
> +		if (ret)
>  			defrag_global_errors++;
> -		}
> +		close_file_or_dir(fd, dirstream);
>  	}
> +
>  	if (defrag_global_errors)
>  		fprintf(stderr, "total %d failures\n", defrag_global_errors);
>  
>

Patch
diff mbox

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 7728430f16a1..17d399d58adf 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -1029,23 +1029,22 @@  static int cmd_filesystem_defrag(int argc, char **argv)
 		if (fd < 0) {
 			error("cannot open %s: %s", argv[i],
 					strerror(errno));
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -errno;
+			goto next;
 		}
-		if (fstat(fd, &st)) {
+
+		ret = fstat(fd, &st);
+		if (ret) {
 			error("failed to stat %s: %s",
 					argv[i], strerror(errno));
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -errno;
+			goto next;
 		}
 		if (!(S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
 			error("%s is not a directory or a regular file",
 					argv[i]);
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -EINVAL;
+			goto next;
 		}
 		if (recursive && S_ISDIR(st.st_mode)) {
 			ret = nftw(argv[i], defrag_callback, 10,
@@ -1060,20 +1059,25 @@  static int cmd_filesystem_defrag(int argc, char **argv)
 			ret = ioctl(fd, BTRFS_IOC_DEFRAG_RANGE,
 					&defrag_global_range);
 			defrag_err = errno;
-		}
-		close_file_or_dir(fd, dirstream);
-		if (ret && defrag_err == ENOTTY) {
-			error(
+			if (ret && defrag_err == ENOTTY) {
+				error(
 "defrag range ioctl not supported in this kernel version, 2.6.33 and newer is required");
-			defrag_global_errors++;
-			break;
+				defrag_global_errors++;
+				close_file_or_dir(fd, dirstream);
+				break;
+			}
+			if (ret) {
+				error("defrag failed on %s: %s", argv[i],
+				      strerror(defrag_err));
+				goto next;
+			}
 		}
-		if (ret) {
-			error("defrag failed on %s: %s", argv[i],
-					strerror(defrag_err));
+next:
+		if (ret)
 			defrag_global_errors++;
-		}
+		close_file_or_dir(fd, dirstream);
 	}
+
 	if (defrag_global_errors)
 		fprintf(stderr, "total %d failures\n", defrag_global_errors);