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

[Boris Burkov <boris@bur.io>]

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
>