diff mbox series

[1/2] btrfs-progs: factor out device stats printing code

Message ID 20220718113439.2997247-1-nborisov@suse.com (mailing list archive)
State New, archived
Headers show
Series [1/2] btrfs-progs: factor out device stats printing code | expand

Commit Message

Nikolay Borisov July 18, 2022, 11:34 a.m. UTC 
This is in preparation for introducing tabular output for device stats. Simply
factor out string-specific output lines in a separate function.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
 1 file changed, 76 insertions(+), 65 deletions(-)

--
2.17.1

Comments

David Sterba July 18, 2022, 4:53 p.m. UTC | #1 
On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 65 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index 7d3febff96c2..feffe9184726 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -577,6 +577,71 @@ static const char * const cmd_device_stats_usage[] = {
>  	NULL
>  };
> 
> +static int _print_device_stat_string(struct format_ctx *fctx,
> +		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
> +{
> +	char *canonical_path = path_canonicalize(path);
> +	char devid_str[32];
> +	int j;
> +	int err = 0;
> +	static const struct {
> +		const char name[32];
> +		enum btrfs_dev_stat_values stat_idx;
> +	} dev_stats[] = {
> +		{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
> +		{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
> +		{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
> +		{ "corruption_errs", BTRFS_DEV_STAT_CORRUPTION_ERRS },
> +		{ "generation_errs", BTRFS_DEV_STAT_GENERATION_ERRS },
> +	};
> +	/*
> +	 * The plain text and json formats cannot be
> +	 * mapped directly in all cases and we have to switch
> +	 */
> +	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> +
> +	/* No path when device is missing. */
> +	if (!canonical_path) {
> +		canonical_path = malloc(32);
> +
> +		if (!canonical_path) {
> +			error("not enough memory for path buffer");
> +			return -ENOMEM;
> +		}
> +
> +		snprintf(canonical_path, 32, "devid:%llu", args->devid);
> +	}
> +	snprintf(devid_str, 32, "%llu", args->devid);
> +	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
> +	/* Plain text does not print device info */
> +	if (json) {
> +		fmt_print(fctx, "device", canonical_path);
> +		fmt_print(fctx, "devid", args->devid);
> +	}
> +
> +	for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> +		enum btrfs_dev_stat_values stat_idx = dev_stats[j].stat_idx;
> +		/* We got fewer items than we know */
> +		if (args->nr_items < stat_idx + 1)
> +			continue;
> +
> +		/* Own format due to [/dev/name].value */
> +		if (json) {
> +			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
> +		} else {
> +			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
> +					(unsigned long long)args->values[stat_idx]);
> +		}
> +		if (check && (args->values[stat_idx] > 0))
> +			err |= 64;
> +	}
> +
> +	fmt_print_end_group(fctx, NULL);
> +	free(canonical_path);
> +
> +	return err;
> +}
> +
>  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  {
>  	char *dev_path;
> @@ -586,7 +651,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  	int fdmnt;
>  	int i;
>  	int err = 0;
> -	int check = 0;
> +	bool check = false;
>  	__u64 flags = 0;
>  	DIR *dirstream = NULL;
>  	struct format_ctx fctx;
> @@ -606,7 +671,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> 
>  		switch (c) {
>  		case 'c':
> -			check = 1;
> +			check = true;
>  			break;
>  		case 'z':
>  			flags = BTRFS_DEV_STATS_RESET;
> @@ -656,70 +721,16 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			error("device stats ioctl failed on %s: %m",
>  			      path);
>  			err |= 1;
> -		} else {
> -			char *canonical_path;
> -			char devid_str[32];
> -			int j;
> -			static const struct {
> -				const char name[32];
> -				u64 num;
> -			} dev_stats[] = {
> -				{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
> -				{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
> -				{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
> -				{ "corruption_errs",
> -					BTRFS_DEV_STAT_CORRUPTION_ERRS },
> -				{ "generation_errs",
> -					BTRFS_DEV_STAT_GENERATION_ERRS },
> -			};
> -			/*
> -			 * The plain text and json formats cannot be
> -			 * mapped directly in all cases and we have to switch
> -			 */
> -			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> -
> -			canonical_path = path_canonicalize(path);
> -
> -			/* No path when device is missing. */
> -			if (!canonical_path) {
> -				canonical_path = malloc(32);
> -				if (!canonical_path) {
> -					error("not enough memory for path buffer");
> -					goto out;
> -				}
> -				snprintf(canonical_path, 32,
> -					 "devid:%llu", args.devid);
> -			}
> -			snprintf(devid_str, 32, "%llu", args.devid);
> -			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
> -			/* Plain text does not print device info */
> -			if (json) {
> -				fmt_print(&fctx, "device", canonical_path);
> -				fmt_print(&fctx, "devid", di_args[i].devid);
> -			}
> +			goto out;
> +		}
> 
> -			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> -				/* We got fewer items than we know */
> -				if (args.nr_items < dev_stats[j].num + 1)
> -					continue;
> -
> -				/* Own format due to [/dev/name].value */
> -				if (json) {
> -					fmt_print(&fctx, dev_stats[j].name,
> -						args.values[dev_stats[j].num]);
> -				} else {
> -					printf("[%s].%-16s %llu\n",
> -						canonical_path,
> -						dev_stats[j].name,
> -						(unsigned long long)
> -						args.values[dev_stats[j].num]);
> -				}
> -				if ((check == 1)
> -				    && (args.values[dev_stats[j].num] > 0))
> -					err |= 64;
> -			}
> -			fmt_print_end_group(&fctx, NULL);
> -			free(canonical_path);
> +		int err2 = _print_device_stat_string(&fctx, &args, path, check);

Please don't define variables in the statement block.

> +		if (err2) {
> +			if (err2 < 0) {
> +				err = err2;
> +				goto out;
> +			} else
> +				err |= err2;
>  		}
>  	}
> 
> --
> 2.17.1
David Sterba July 18, 2022, 5 p.m. UTC | #2 
On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Added to devel, with some fixups, thanks.
Boris Burkov July 18, 2022, 5:11 p.m. UTC | #3 
On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.

LGTM, and works fine. I mentioned a few nits inline.

> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 65 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index 7d3febff96c2..feffe9184726 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -577,6 +577,71 @@ static const char * const cmd_device_stats_usage[] = {
>  	NULL
>  };
> 

Documenting the return value seems valuable, since it has different
semantics for positive/negative

> +static int _print_device_stat_string(struct format_ctx *fctx,
> +		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
> +{
> +	char *canonical_path = path_canonicalize(path);
> +	char devid_str[32];
> +	int j;
> +	int err = 0;
> +	static const struct {
> +		const char name[32];
> +		enum btrfs_dev_stat_values stat_idx;
> +	} dev_stats[] = {
> +		{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
> +		{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
> +		{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
> +		{ "corruption_errs", BTRFS_DEV_STAT_CORRUPTION_ERRS },
> +		{ "generation_errs", BTRFS_DEV_STAT_GENERATION_ERRS },
> +	};
> +	/*
> +	 * The plain text and json formats cannot be
> +	 * mapped directly in all cases and we have to switch
> +	 */
> +	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> +
> +	/* No path when device is missing. */
> +	if (!canonical_path) {
> +		canonical_path = malloc(32);
> +
> +		if (!canonical_path) {
> +			error("not enough memory for path buffer");
> +			return -ENOMEM;

I believe the old code didn't actually set err to ENOMEM in this case. I
assume this is an improvement, but figured it was worth noting.

> +		}
> +
> +		snprintf(canonical_path, 32, "devid:%llu", args->devid);
> +	}
> +	snprintf(devid_str, 32, "%llu", args->devid);
> +	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
> +	/* Plain text does not print device info */
> +	if (json) {
> +		fmt_print(fctx, "device", canonical_path);
> +		fmt_print(fctx, "devid", args->devid);
> +	}
> +
> +	for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> +		enum btrfs_dev_stat_values stat_idx = dev_stats[j].stat_idx;
> +		/* We got fewer items than we know */
> +		if (args->nr_items < stat_idx + 1)
> +			continue;
> +
> +		/* Own format due to [/dev/name].value */
> +		if (json) {
> +			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
> +		} else {
> +			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
> +					(unsigned long long)args->values[stat_idx]);
> +		}
> +		if (check && (args->values[stat_idx] > 0))
> +			err |= 64;

now that err starts at zero and gets returned, |= doesn't really do
anything here, compared to just =, does it?

> +	}
> +
> +	fmt_print_end_group(fctx, NULL);
> +	free(canonical_path);
> +
> +	return err;
> +}
> +
>  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  {
>  	char *dev_path;
> @@ -586,7 +651,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  	int fdmnt;
>  	int i;
>  	int err = 0;
> -	int check = 0;
> +	bool check = false;
>  	__u64 flags = 0;
>  	DIR *dirstream = NULL;
>  	struct format_ctx fctx;
> @@ -606,7 +671,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> 
>  		switch (c) {
>  		case 'c':
> -			check = 1;
> +			check = true;
>  			break;
>  		case 'z':
>  			flags = BTRFS_DEV_STATS_RESET;
> @@ -656,70 +721,16 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			error("device stats ioctl failed on %s: %m",
>  			      path);
>  			err |= 1;
> -		} else {
> -			char *canonical_path;
> -			char devid_str[32];
> -			int j;
> -			static const struct {
> -				const char name[32];
> -				u64 num;
> -			} dev_stats[] = {
> -				{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
> -				{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
> -				{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
> -				{ "corruption_errs",
> -					BTRFS_DEV_STAT_CORRUPTION_ERRS },
> -				{ "generation_errs",
> -					BTRFS_DEV_STAT_GENERATION_ERRS },
> -			};
> -			/*
> -			 * The plain text and json formats cannot be
> -			 * mapped directly in all cases and we have to switch
> -			 */
> -			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> -
> -			canonical_path = path_canonicalize(path);
> -
> -			/* No path when device is missing. */
> -			if (!canonical_path) {
> -				canonical_path = malloc(32);
> -				if (!canonical_path) {
> -					error("not enough memory for path buffer");
> -					goto out;
> -				}
> -				snprintf(canonical_path, 32,
> -					 "devid:%llu", args.devid);
> -			}
> -			snprintf(devid_str, 32, "%llu", args.devid);
> -			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
> -			/* Plain text does not print device info */
> -			if (json) {
> -				fmt_print(&fctx, "device", canonical_path);
> -				fmt_print(&fctx, "devid", di_args[i].devid);
> -			}
> +			goto out;
> +		}
> 
> -			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> -				/* We got fewer items than we know */
> -				if (args.nr_items < dev_stats[j].num + 1)
> -					continue;
> -
> -				/* Own format due to [/dev/name].value */
> -				if (json) {
> -					fmt_print(&fctx, dev_stats[j].name,
> -						args.values[dev_stats[j].num]);
> -				} else {
> -					printf("[%s].%-16s %llu\n",
> -						canonical_path,
> -						dev_stats[j].name,
> -						(unsigned long long)
> -						args.values[dev_stats[j].num]);
> -				}
> -				if ((check == 1)
> -				    && (args.values[dev_stats[j].num] > 0))
> -					err |= 64;
> -			}
> -			fmt_print_end_group(&fctx, NULL);
> -			free(canonical_path);
> +		int err2 = _print_device_stat_string(&fctx, &args, path, check);
> +		if (err2) {
> +			if (err2 < 0) {
> +				err = err2;
> +				goto out;
> +			} else
> +				err |= err2;
>  		}
>  	}
> 
> --
> 2.17.1
>
diff mbox series

Patch

diff --git a/cmds/device.c b/cmds/device.c
index 7d3febff96c2..feffe9184726 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -577,6 +577,71 @@  static const char * const cmd_device_stats_usage[] = {
 	NULL
 };

+static int _print_device_stat_string(struct format_ctx *fctx,
+		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
+{
+	char *canonical_path = path_canonicalize(path);
+	char devid_str[32];
+	int j;
+	int err = 0;
+	static const struct {
+		const char name[32];
+		enum btrfs_dev_stat_values stat_idx;
+	} dev_stats[] = {
+		{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
+		{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
+		{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
+		{ "corruption_errs", BTRFS_DEV_STAT_CORRUPTION_ERRS },
+		{ "generation_errs", BTRFS_DEV_STAT_GENERATION_ERRS },
+	};
+	/*
+	 * The plain text and json formats cannot be
+	 * mapped directly in all cases and we have to switch
+	 */
+	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
+
+	/* No path when device is missing. */
+	if (!canonical_path) {
+		canonical_path = malloc(32);
+
+		if (!canonical_path) {
+			error("not enough memory for path buffer");
+			return -ENOMEM;
+		}
+
+		snprintf(canonical_path, 32, "devid:%llu", args->devid);
+	}
+	snprintf(devid_str, 32, "%llu", args->devid);
+	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
+	/* Plain text does not print device info */
+	if (json) {
+		fmt_print(fctx, "device", canonical_path);
+		fmt_print(fctx, "devid", args->devid);
+	}
+
+	for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
+		enum btrfs_dev_stat_values stat_idx = dev_stats[j].stat_idx;
+		/* We got fewer items than we know */
+		if (args->nr_items < stat_idx + 1)
+			continue;
+
+		/* Own format due to [/dev/name].value */
+		if (json) {
+			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
+		} else {
+			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
+					(unsigned long long)args->values[stat_idx]);
+		}
+		if (check && (args->values[stat_idx] > 0))
+			err |= 64;
+	}
+
+	fmt_print_end_group(fctx, NULL);
+	free(canonical_path);
+
+	return err;
+}
+
 static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	char *dev_path;
@@ -586,7 +651,7 @@  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 	int fdmnt;
 	int i;
 	int err = 0;
-	int check = 0;
+	bool check = false;
 	__u64 flags = 0;
 	DIR *dirstream = NULL;
 	struct format_ctx fctx;
@@ -606,7 +671,7 @@  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)

 		switch (c) {
 		case 'c':
-			check = 1;
+			check = true;
 			break;
 		case 'z':
 			flags = BTRFS_DEV_STATS_RESET;
@@ -656,70 +721,16 @@  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 			error("device stats ioctl failed on %s: %m",
 			      path);
 			err |= 1;
-		} else {
-			char *canonical_path;
-			char devid_str[32];
-			int j;
-			static const struct {
-				const char name[32];
-				u64 num;
-			} dev_stats[] = {
-				{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
-				{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
-				{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
-				{ "corruption_errs",
-					BTRFS_DEV_STAT_CORRUPTION_ERRS },
-				{ "generation_errs",
-					BTRFS_DEV_STAT_GENERATION_ERRS },
-			};
-			/*
-			 * The plain text and json formats cannot be
-			 * mapped directly in all cases and we have to switch
-			 */
-			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
-
-			canonical_path = path_canonicalize(path);
-
-			/* No path when device is missing. */
-			if (!canonical_path) {
-				canonical_path = malloc(32);
-				if (!canonical_path) {
-					error("not enough memory for path buffer");
-					goto out;
-				}
-				snprintf(canonical_path, 32,
-					 "devid:%llu", args.devid);
-			}
-			snprintf(devid_str, 32, "%llu", args.devid);
-			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
-			/* Plain text does not print device info */
-			if (json) {
-				fmt_print(&fctx, "device", canonical_path);
-				fmt_print(&fctx, "devid", di_args[i].devid);
-			}
+			goto out;
+		}

-			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
-				/* We got fewer items than we know */
-				if (args.nr_items < dev_stats[j].num + 1)
-					continue;
-
-				/* Own format due to [/dev/name].value */
-				if (json) {
-					fmt_print(&fctx, dev_stats[j].name,
-						args.values[dev_stats[j].num]);
-				} else {
-					printf("[%s].%-16s %llu\n",
-						canonical_path,
-						dev_stats[j].name,
-						(unsigned long long)
-						args.values[dev_stats[j].num]);
-				}
-				if ((check == 1)
-				    && (args.values[dev_stats[j].num] > 0))
-					err |= 64;
-			}
-			fmt_print_end_group(&fctx, NULL);
-			free(canonical_path);
+		int err2 = _print_device_stat_string(&fctx, &args, path, check);
+		if (err2) {
+			if (err2 < 0) {
+				err = err2;
+				goto out;
+			} else
+				err |= err2;
 		}
 	}