[PATCH v3 1/2] btrfs-progs: common: extend fmt_print_start_group handles unnamed group

[PATCH v3 1/2] btrfs-progs: common: extend fmt_print_start_group handles unnamed group

[Sidong Yang]

This patch extends fmt_print_start_group() that it can handle when name
argument is NULL. It is useful for printing unnamed array or map.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
v3:
 - extend fmt_print_start_group rather than writing new function
---
 common/format-output.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/common/format-output.c b/common/format-output.c
index 8df93ecb..2f74595c 100644
--- a/common/format-output.c
+++ b/common/format-output.c
@@ -181,17 +181,23 @@ void fmt_end_value(struct format_ctx *fctx, const struct rowspec *row)
 void fmt_print_start_group(struct format_ctx *fctx, const char *name,
 		enum json_type jtype)
 {
+	char bracket;
 	if (bconf.output_format == CMD_FORMAT_JSON) {
 		fmt_separator(fctx);
 		fmt_inc_depth(fctx);
 		fctx->jtype[fctx->depth] = jtype;
 		fctx->memb[fctx->depth] = 0;
 		if (jtype == JSON_TYPE_MAP)
-			printf("\"%s\": {", name);
+			bracket = '{';
 		else if (jtype == JSON_TYPE_ARRAY)
-			printf("\"%s\": [", name);
+			bracket = '[';
 		else
 			fmt_error(fctx);
+
+		if (name)
+			printf("\"%s\": %c", name, bracket);
+		else
+			putchar(bracket);
 	}
 }
 
-- 
2.25.1

[PATCH v3 2/2] btrfs-progs: device stats: add json output format

[Sidong Yang]

Add supports for json formatting, this patch changes hard coded printing
code to formatted print with output formatter. Json output would be
useful for other programs that parse output of the command. but it
changes the text format.

Example text format:

device:                 /dev/vdb
devid			1
write_io_errs:          0
read_io_errs:           0
flush_io_errs:          0
corruption_errs:        0
generation_errs:        0

Example json format:

{
  "__header": {
    "version": "1"
  },
  "device-stats": [
    {
      "device": "/dev/vdb",
      "devid": "1",
      "write_io_errs": "0",
      "read_io_errs": "0",
      "flush_io_errs": "0",
      "corruption_errs": "0",
      "generation_errs": "0"
    }
  ],
}

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
v3:
 - use fmt_print_start_group with NULL name
---
 cmds/device.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/cmds/device.c b/cmds/device.c
index d72881f8..204e834b 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -36,6 +36,7 @@
 #include "common/path-utils.h"
 #include "common/device-utils.h"
 #include "common/device-scan.h"
+#include "common/format-output.h"
 #include "mkfs/common.h"
 
 static const char * const device_cmd_group_usage[] = {
@@ -459,6 +460,17 @@ out:
 }
 static DEFINE_SIMPLE_COMMAND(device_ready, "ready");
 
+static const struct rowspec device_stats_rowspec[] = {
+	{ .key = "device", .fmt = "%s", .out_text = "device", .out_json = "device" },
+	{ .key = "devid", .fmt = "%u", .out_text = "devid", .out_json = "devid" },
+	{ .key = "write_io_errs", .fmt = "%llu", .out_text = "write_io_errs", .out_json = "write_io_errs" },
+	{ .key = "read_io_errs", .fmt = "%llu", .out_text = "read_io_errs", .out_json = "read_io_errs" },
+	{ .key = "flush_io_errs", .fmt = "%llu", .out_text = "flush_io_errs", .out_json = "flush_io_errs" },
+	{ .key = "corruption_errs", .fmt = "%llu", .out_text = "corruption_errs", .out_json = "corruption_errs" },
+	{ .key = "generation_errs", .fmt = "%llu", .out_text = "generation_errs", .out_json = "generation_errs" },
+	ROWSPEC_END
+};
+
 static const char * const cmd_device_stats_usage[] = {
 	"btrfs device stats [options] <path>|<device>",
 	"Show device IO error statistics",
@@ -482,6 +494,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 	int check = 0;
 	__u64 flags = 0;
 	DIR *dirstream = NULL;
+	struct format_ctx fctx;
 
 	optind = 0;
 	while (1) {
@@ -530,6 +543,8 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 		goto out;
 	}
 
+	fmt_start(&fctx, device_stats_rowspec, 24, 0);
+	fmt_print_start_group(&fctx, "device-stats", JSON_TYPE_ARRAY);
 	for (i = 0; i < fi_args.num_devices; i++) {
 		struct btrfs_ioctl_get_dev_stats args = {0};
 		char path[BTRFS_DEVICE_PATH_NAME_MAX + 1];
@@ -548,6 +563,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 			err |= 1;
 		} else {
 			char *canonical_path;
+			char devid_str[32];
 			int j;
 			static const struct {
 				const char name[32];
@@ -574,31 +590,36 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 				snprintf(canonical_path, 32,
 					 "devid:%llu", args.devid);
 			}
+			snprintf(devid_str, 32, "%llu", args.devid);
+			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
+			fmt_print(&fctx, "device", canonical_path);
+			fmt_print(&fctx, "devid", di_args[i].devid);
 
 			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;
-				printf("[%s].%-16s %llu\n", canonical_path,
-					dev_stats[j].name,
-					(unsigned long long)
-					 args.values[dev_stats[j].num]);
+
+				fmt_print(&fctx, dev_stats[j].name, 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);
 		}
 	}
 
+	fmt_print_end_group(&fctx, "device-stats");
+	fmt_end(&fctx);
+
 out:
 	free(di_args);
 	close_file_or_dir(fdmnt, dirstream);
 
 	return err;
 }
-static DEFINE_SIMPLE_COMMAND(device_stats, "stats");
+static DEFINE_COMMAND_WITH_FLAGS(device_stats, "stats", CMD_FORMAT_JSON);
 
 static const char * const cmd_device_usage_usage[] = {
 	"btrfs device usage [options] <path> [<path>..]",
-- 
2.25.1

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> Add supports for json formatting, this patch changes hard coded printing
> code to formatted print with output formatter. Json output would be
> useful for other programs that parse output of the command. but it
> changes the text format.

I did not realize that before, but we can't change the output like that.
That would break fstests.

> Example text format:
> 
> device:                 /dev/vdb
> devid			1
> write_io_errs:          0
> read_io_errs:           0
> flush_io_errs:          0
> corruption_errs:        0
> generation_errs:        0

But at least it's just one more switch that keeps the printf and json
format inside the loop, the rest can stay.

> Example json format:
> 
> {
>   "__header": {
>     "version": "1"
>   },
>   "device-stats": [
>     {
>       "device": "/dev/vdb",
>       "devid": "1",
>       "write_io_errs": "0",
>       "read_io_errs": "0",
>       "flush_io_errs": "0",
>       "corruption_errs": "0",
>       "generation_errs": "0"
>     }
>   ],
     ^

I've verified that the comma is really there, it's not a valid json so
there's a bug in the formatter. To verify that the output is valid you
can use eg. 'jq', simply pipe the output of the commadn there.

  $ ./btrfs --format json dev stats /mnt | jq
  parse error: Expected another key-value pair at line 16, column 1

> }
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
> v3:
>  - use fmt_print_start_group with NULL name
> ---
>  cmds/device.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index d72881f8..204e834b 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -36,6 +36,7 @@
>  #include "common/path-utils.h"
>  #include "common/device-utils.h"
>  #include "common/device-scan.h"
> +#include "common/format-output.h"
>  #include "mkfs/common.h"
>  
>  static const char * const device_cmd_group_usage[] = {
> @@ -459,6 +460,17 @@ out:
>  }
>  static DEFINE_SIMPLE_COMMAND(device_ready, "ready");
>  
> +static const struct rowspec device_stats_rowspec[] = {
> +	{ .key = "device", .fmt = "%s", .out_text = "device", .out_json = "device" },
> +	{ .key = "devid", .fmt = "%u", .out_text = "devid", .out_json = "devid" },
> +	{ .key = "write_io_errs", .fmt = "%llu", .out_text = "write_io_errs", .out_json = "write_io_errs" },
> +	{ .key = "read_io_errs", .fmt = "%llu", .out_text = "read_io_errs", .out_json = "read_io_errs" },
> +	{ .key = "flush_io_errs", .fmt = "%llu", .out_text = "flush_io_errs", .out_json = "flush_io_errs" },
> +	{ .key = "corruption_errs", .fmt = "%llu", .out_text = "corruption_errs", .out_json = "corruption_errs" },
> +	{ .key = "generation_errs", .fmt = "%llu", .out_text = "generation_errs", .out_json = "generation_errs" },
> +	ROWSPEC_END
> +};
> +
>  static const char * const cmd_device_stats_usage[] = {
>  	"btrfs device stats [options] <path>|<device>",
>  	"Show device IO error statistics",

The usage help text should also advertise the formats, so this needs the
helpinfo stubs:

--- a/cmds/device.c
+++ b/cmds/device.c
@@ -527,6 +527,8 @@ static const char * const cmd_device_stats_usage[] = {
        "",
        "-c|--check             return non-zero if any stat counter is not zero",
        "-z|--reset             show current stats and reset values to zero",
+       HELPINFO_INSERT_GLOBALS,
+       HELPINFO_INSERT_FORMAT,
        NULL
 };
---

> @@ -482,6 +494,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  	int check = 0;
>  	__u64 flags = 0;
>  	DIR *dirstream = NULL;
> +	struct format_ctx fctx;
>  
>  	optind = 0;
>  	while (1) {
> @@ -530,6 +543,8 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  		goto out;
>  	}
>  
> +	fmt_start(&fctx, device_stats_rowspec, 24, 0);
> +	fmt_print_start_group(&fctx, "device-stats", JSON_TYPE_ARRAY);
>  	for (i = 0; i < fi_args.num_devices; i++) {
>  		struct btrfs_ioctl_get_dev_stats args = {0};
>  		char path[BTRFS_DEVICE_PATH_NAME_MAX + 1];
> @@ -548,6 +563,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			err |= 1;
>  		} else {
>  			char *canonical_path;
> +			char devid_str[32];
>  			int j;
>  			static const struct {
>  				const char name[32];
> @@ -574,31 +590,36 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  				snprintf(canonical_path, 32,
>  					 "devid:%llu", args.devid);
>  			}
> +			snprintf(devid_str, 32, "%llu", args.devid);
> +			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
> +			fmt_print(&fctx, "device", canonical_path);
> +			fmt_print(&fctx, "devid", di_args[i].devid);
>  
>  			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;
> -				printf("[%s].%-16s %llu\n", canonical_path,
> -					dev_stats[j].name,
> -					(unsigned long long)
> -					 args.values[dev_stats[j].num]);
> +
> +				fmt_print(&fctx, dev_stats[j].name, args.values[dev_stats[j].num]);

So the switch goes here

>  				if ((check == 1)
>  				    && (args.values[dev_stats[j].num] > 0))
>  					err |= 64;
>  			}
> -
> +			fmt_print_end_group(&fctx, NULL);
>  			free(canonical_path);
>  		}
>  	}
>  
> +	fmt_print_end_group(&fctx, "device-stats");
> +	fmt_end(&fctx);
> +
>  out:
>  	free(di_args);
>  	close_file_or_dir(fdmnt, dirstream);
>  
>  	return err;
>  }
> -static DEFINE_SIMPLE_COMMAND(device_stats, "stats");
> +static DEFINE_COMMAND_WITH_FLAGS(device_stats, "stats", CMD_FORMAT_JSON);
>  
>  static const char * const cmd_device_usage_usage[] = {
>  	"btrfs device usage [options] <path> [<path>..]",
> -- 
> 2.25.1

Re: [PATCH v3 1/2] btrfs-progs: common: extend fmt_print_start_group handles unnamed group

[David Sterba]

On Fri, Dec 11, 2020 at 04:48:11PM +0000, Sidong Yang wrote:
> This patch extends fmt_print_start_group() that it can handle when name
> argument is NULL. It is useful for printing unnamed array or map.
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
> v3:
>  - extend fmt_print_start_group rather than writing new function
> ---
>  common/format-output.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/common/format-output.c b/common/format-output.c
> index 8df93ecb..2f74595c 100644
> --- a/common/format-output.c
> +++ b/common/format-output.c
> @@ -181,17 +181,23 @@ void fmt_end_value(struct format_ctx *fctx, const struct rowspec *row)
>  void fmt_print_start_group(struct format_ctx *fctx, const char *name,
>  		enum json_type jtype)
>  {
> +	char bracket;
>  	if (bconf.output_format == CMD_FORMAT_JSON) {
>  		fmt_separator(fctx);
>  		fmt_inc_depth(fctx);
>  		fctx->jtype[fctx->depth] = jtype;
>  		fctx->memb[fctx->depth] = 0;

This can be simplified a bit, the name can be conditionally printed here

>  		if (jtype == JSON_TYPE_MAP)
> -			printf("\"%s\": {", name);
> +			bracket = '{';

and this just does the right putchar(). With this change now added to
devel, thanks.

>  		else if (jtype == JSON_TYPE_ARRAY)
> -			printf("\"%s\": [", name);
> +			bracket = '[';
>  		else
>  			fmt_error(fctx);
> +
> +		if (name)
> +			printf("\"%s\": %c", name, bracket);
> +		else
> +			putchar(bracket);

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > Example json format:
> > 
> > {
> >   "__header": {
> >     "version": "1"
> >   },
> >   "device-stats": [
> >     {
> >       "device": "/dev/vdb",
> >       "devid": "1",
> >       "write_io_errs": "0",
> >       "read_io_errs": "0",
> >       "flush_io_errs": "0",
> >       "corruption_errs": "0",
> >       "generation_errs": "0"
> >     }
> >   ],
>      ^
> 
> I've verified that the comma is really there, it's not a valid json so
> there's a bug in the formatter. To verify that the output is valid you
> can use eg. 'jq', simply pipe the output of the commadn there.
> 
>   $ ./btrfs --format json dev stats /mnt | jq
>   parse error: Expected another key-value pair at line 16, column 1

I've pushed the updated plain text formatting to devel, so the only
remaining bug is the above extra comma.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[Sidong Yang]

On Fri, Dec 11, 2020 at 06:46:29PM +0100, David Sterba wrote:
> On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > Example json format:
> > > 
> > > {
> > >   "__header": {
> > >     "version": "1"
> > >   },
> > >   "device-stats": [
> > >     {
> > >       "device": "/dev/vdb",
> > >       "devid": "1",
> > >       "write_io_errs": "0",
> > >       "read_io_errs": "0",
> > >       "flush_io_errs": "0",
> > >       "corruption_errs": "0",
> > >       "generation_errs": "0"
> > >     }
> > >   ],
> >      ^
> > 
> > I've verified that the comma is really there, it's not a valid json so
> > there's a bug in the formatter. To verify that the output is valid you
> > can use eg. 'jq', simply pipe the output of the commadn there.
> > 
> >   $ ./btrfs --format json dev stats /mnt | jq
> >   parse error: Expected another key-value pair at line 16, column 1
> 
> I've pushed the updated plain text formatting to devel, so the only
> remaining bug is the above extra comma.

I've tested devel branch. but there is no extra comma and I also tested
with jq you said. jq results no error. I think that It's just mistype in
message.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[Su Yue]

On Sat, Dec 12, 2020 at 3:04 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > Example json format:
> > >
> > > {
> > >   "__header": {
> > >     "version": "1"
> > >   },
> > >   "device-stats": [
> > >     {
> > >       "device": "/dev/vdb",
> > >       "devid": "1",
> > >       "write_io_errs": "0",
> > >       "read_io_errs": "0",
> > >       "flush_io_errs": "0",
> > >       "corruption_errs": "0",
> > >       "generation_errs": "0"
> > >     }
> > >   ],
> >      ^
> >
> > I've verified that the comma is really there, it's not a valid json so
> > there's a bug in the formatter. To verify that the output is valid you
> > can use eg. 'jq', simply pipe the output of the commadn there.
> >
> >   $ ./btrfs --format json dev stats /mnt | jq
> >   parse error: Expected another key-value pair at line 16, column 1
>
> I've pushed the updated plain text formatting to devel, so the only
> remaining bug is the above extra comma.

Another format bug(one extra newline):
========================================
➜  btrfs-progs git:(314d96c8)  btrfs device stats /mnt
[/dev/mapper/test-1].write_io_errs    0
[/dev/mapper/test-1].read_io_errs     0
[/dev/mapper/test-1].flush_io_errs    0
[/dev/mapper/test-1].corruption_errs  0
[/dev/mapper/test-1].generation_errs  0

➜  btrfs-progs git:(314d96c8)
========================================
The new line is printed by the change:
'+       fmt_end(&fctx);'

and fstests/btrfs/006 fails:
================================================================================
btrfs/006 1s ... - output mismatch (see
/root/xfstests-dev/results//btrfs/006.out.bad)
    --- tests/btrfs/006.out     2020-12-16 03:40:19.632039261 +0000
    +++ /root/xfstests-dev/results//btrfs/006.out.bad   2020-12-16
06:25:56.424424113 +0000
    @@ -15,12 +15,14 @@

     == Sync filesystem
     == Show device stats by mountpoint
    + 1
     <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM>
     <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM>
     <NUMDEVS> [SCRATCH_DEV].generation_errs <NUM>
    ...
    (Run 'diff -u /root/xfstests-dev/tests/btrfs/006.out
/root/xfstests-dev/results//btrfs/006.out.bad'  to see the entire
diff)
Ran: btrfs/006
Failures: btrfs/006
Failed 1 of 1 tests
================================================================================

The new line made filter produce the '+1'.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[Sidong Yang]

On Wed, Dec 16, 2020 at 02:30:04PM +0800, Su Yue wrote:
> On Sat, Dec 12, 2020 at 3:04 AM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > > Example json format:
> > > >
> > > > {
> > > >   "__header": {
> > > >     "version": "1"
> > > >   },
> > > >   "device-stats": [
> > > >     {
> > > >       "device": "/dev/vdb",
> > > >       "devid": "1",
> > > >       "write_io_errs": "0",
> > > >       "read_io_errs": "0",
> > > >       "flush_io_errs": "0",
> > > >       "corruption_errs": "0",
> > > >       "generation_errs": "0"
> > > >     }
> > > >   ],
> > >      ^
> > >
> > > I've verified that the comma is really there, it's not a valid json so
> > > there's a bug in the formatter. To verify that the output is valid you
> > > can use eg. 'jq', simply pipe the output of the commadn there.
> > >
> > >   $ ./btrfs --format json dev stats /mnt | jq
> > >   parse error: Expected another key-value pair at line 16, column 1
> >
> > I've pushed the updated plain text formatting to devel, so the only
> > remaining bug is the above extra comma.
> 
> Another format bug(one extra newline):
> ========================================
> ➜  btrfs-progs git:(314d96c8)  btrfs device stats /mnt
> [/dev/mapper/test-1].write_io_errs    0
> [/dev/mapper/test-1].read_io_errs     0
> [/dev/mapper/test-1].flush_io_errs    0
> [/dev/mapper/test-1].corruption_errs  0
> [/dev/mapper/test-1].generation_errs  0
> 
> ➜  btrfs-progs git:(314d96c8)
> ========================================
> The new line is printed by the change:
> '+       fmt_end(&fctx);'
> 
> and fstests/btrfs/006 fails:
> ================================================================================
> btrfs/006 1s ... - output mismatch (see
> /root/xfstests-dev/results//btrfs/006.out.bad)
>     --- tests/btrfs/006.out     2020-12-16 03:40:19.632039261 +0000
>     +++ /root/xfstests-dev/results//btrfs/006.out.bad   2020-12-16
> 06:25:56.424424113 +0000
>     @@ -15,12 +15,14 @@
> 
>      == Sync filesystem
>      == Show device stats by mountpoint
>     + 1
>      <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM>
>      <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM>
>      <NUMDEVS> [SCRATCH_DEV].generation_errs <NUM>
>     ...
>     (Run 'diff -u /root/xfstests-dev/tests/btrfs/006.out
> /root/xfstests-dev/results//btrfs/006.out.bad'  to see the entire
> diff)
> Ran: btrfs/006
> Failures: btrfs/006
> Failed 1 of 1 tests
> ================================================================================
> 
> The new line made filter produce the '+1'.

Thanks for testing this patch.
I checked the fmt_end() and there is an additional newline.
I think that fmt_end() should be used for formatting. so it seems that
the only way to fix this problem is to remove the code that inserts a
newline in fmt_end(). I searched the code that use the function and
there is no code that used this function but this patch. Do you have any
ideas?

Thanks,
Sidong

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[Su Yue]

On Wed, Dec 16, 2020 at 6:52 PM Sidong Yang <realwakka@gmail.com> wrote:
>
> On Wed, Dec 16, 2020 at 02:30:04PM +0800, Su Yue wrote:
> > On Sat, Dec 12, 2020 at 3:04 AM David Sterba <dsterba@suse.cz> wrote:
> > >
> > > On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > > > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > > > Example json format:
> > > > >
> > > > > {
> > > > >   "__header": {
> > > > >     "version": "1"
> > > > >   },
> > > > >   "device-stats": [
> > > > >     {
> > > > >       "device": "/dev/vdb",
> > > > >       "devid": "1",
> > > > >       "write_io_errs": "0",
> > > > >       "read_io_errs": "0",
> > > > >       "flush_io_errs": "0",
> > > > >       "corruption_errs": "0",
> > > > >       "generation_errs": "0"
> > > > >     }
> > > > >   ],
> > > >      ^
> > > >
> > > > I've verified that the comma is really there, it's not a valid json so
> > > > there's a bug in the formatter. To verify that the output is valid you
> > > > can use eg. 'jq', simply pipe the output of the commadn there.
> > > >
> > > >   $ ./btrfs --format json dev stats /mnt | jq
> > > >   parse error: Expected another key-value pair at line 16, column 1
> > >
> > > I've pushed the updated plain text formatting to devel, so the only
> > > remaining bug is the above extra comma.
> >
> > Another format bug(one extra newline):
> > ========================================
> > ➜  btrfs-progs git:(314d96c8)  btrfs device stats /mnt
> > [/dev/mapper/test-1].write_io_errs    0
> > [/dev/mapper/test-1].read_io_errs     0
> > [/dev/mapper/test-1].flush_io_errs    0
> > [/dev/mapper/test-1].corruption_errs  0
> > [/dev/mapper/test-1].generation_errs  0
> >
> > ➜  btrfs-progs git:(314d96c8)
> > ========================================
> > The new line is printed by the change:
> > '+       fmt_end(&fctx);'
> >
> > and fstests/btrfs/006 fails:
> > ================================================================================
> > btrfs/006 1s ... - output mismatch (see
> > /root/xfstests-dev/results//btrfs/006.out.bad)
> >     --- tests/btrfs/006.out     2020-12-16 03:40:19.632039261 +0000
> >     +++ /root/xfstests-dev/results//btrfs/006.out.bad   2020-12-16
> > 06:25:56.424424113 +0000
> >     @@ -15,12 +15,14 @@
> >
> >      == Sync filesystem
> >      == Show device stats by mountpoint
> >     + 1
> >      <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM>
> >      <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM>
> >      <NUMDEVS> [SCRATCH_DEV].generation_errs <NUM>
> >     ...
> >     (Run 'diff -u /root/xfstests-dev/tests/btrfs/006.out
> > /root/xfstests-dev/results//btrfs/006.out.bad'  to see the entire
> > diff)
> > Ran: btrfs/006
> > Failures: btrfs/006
> > Failed 1 of 1 tests
> > ================================================================================
> >
> > The new line made filter produce the '+1'.
>
> Thanks for testing this patch.
> I checked the fmt_end() and there is an additional newline.
> I think that fmt_end() should be used for formatting. so it seems that

Yes, it's for the purpose of formatting.

> the only way to fix this problem is to remove the code that inserts a
> newline in fmt_end(). I searched the code that use the function and
> there is no code that used this function but this patch. Do you have any
> ideas?
>
I'm OK about removing the "putchar('\n');". It's just a tiny format issue so no
bother to do extra works.

David?

> Thanks,
> Sidong

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Wed, Dec 16, 2020 at 02:30:04PM +0800, Su Yue wrote:
> On Sat, Dec 12, 2020 at 3:04 AM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > > Example json format:
> > > >
> > > > {
> > > >   "__header": {
> > > >     "version": "1"
> > > >   },
> > > >   "device-stats": [
> > > >     {
> > > >       "device": "/dev/vdb",
> > > >       "devid": "1",
> > > >       "write_io_errs": "0",
> > > >       "read_io_errs": "0",
> > > >       "flush_io_errs": "0",
> > > >       "corruption_errs": "0",
> > > >       "generation_errs": "0"
> > > >     }
> > > >   ],
> > >      ^
> > >
> > > I've verified that the comma is really there, it's not a valid json so
> > > there's a bug in the formatter. To verify that the output is valid you
> > > can use eg. 'jq', simply pipe the output of the commadn there.
> > >
> > >   $ ./btrfs --format json dev stats /mnt | jq
> > >   parse error: Expected another key-value pair at line 16, column 1
> >
> > I've pushed the updated plain text formatting to devel, so the only
> > remaining bug is the above extra comma.
> 
> Another format bug(one extra newline):
> ========================================
> ➜  btrfs-progs git:(314d96c8)  btrfs device stats /mnt
> [/dev/mapper/test-1].write_io_errs    0
> [/dev/mapper/test-1].read_io_errs     0
> [/dev/mapper/test-1].flush_io_errs    0
> [/dev/mapper/test-1].corruption_errs  0
> [/dev/mapper/test-1].generation_errs  0
> 
> ➜  btrfs-progs git:(314d96c8)
> ========================================
> The new line is printed by the change:
> '+       fmt_end(&fctx);'
> 
> and fstests/btrfs/006 fails:
> ================================================================================
> btrfs/006 1s ... - output mismatch (see
> /root/xfstests-dev/results//btrfs/006.out.bad)
>     --- tests/btrfs/006.out     2020-12-16 03:40:19.632039261 +0000
>     +++ /root/xfstests-dev/results//btrfs/006.out.bad   2020-12-16
> 06:25:56.424424113 +0000
>     @@ -15,12 +15,14 @@
> 
>      == Sync filesystem
>      == Show device stats by mountpoint
>     + 1
>      <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM>
>      <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM>
>      <NUMDEVS> [SCRATCH_DEV].generation_errs <NUM>
>     ...
>     (Run 'diff -u /root/xfstests-dev/tests/btrfs/006.out
> /root/xfstests-dev/results//btrfs/006.out.bad'  to see the entire
> diff)
> Ran: btrfs/006
> Failures: btrfs/006
> Failed 1 of 1 tests
> ================================================================================
> 
> The new line made filter produce the '+1'.

I noticed the newline but did not think it would break fstests, so it
needs to be removed.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Wed, Dec 16, 2020 at 08:52:08PM +0800, Su Yue wrote:
> > > The new line made filter produce the '+1'.
> >
> > Thanks for testing this patch.
> > I checked the fmt_end() and there is an additional newline.
> > I think that fmt_end() should be used for formatting. so it seems that
> 
> Yes, it's for the purpose of formatting.
> 
> > the only way to fix this problem is to remove the code that inserts a
> > newline in fmt_end(). I searched the code that use the function and
> > there is no code that used this function but this patch. Do you have any
> > ideas?
> >
> I'm OK about removing the "putchar('\n');". It's just a tiny format issue so no
> bother to do extra works.

The way the json formatting works the newline must be printed from
there. The problematic requirement is delayed insertion of the "," when
there are more objects on the same level. But we don't know that until
the next object is processed.

Mixing the textual and json output for the stats makes it a bit more
complicated as it prints the newline for each row unconditionally,
breaking the assumption of the formatter.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Fri, Dec 11, 2020 at 06:09:11PM +0000, Sidong Yang wrote:
> On Fri, Dec 11, 2020 at 06:46:29PM +0100, David Sterba wrote:
> > On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > > Example json format:
> > > > 
> > > > {
> > > >   "__header": {
> > > >     "version": "1"
> > > >   },
> > > >   "device-stats": [
> > > >     {
> > > >       "device": "/dev/vdb",
> > > >       "devid": "1",
> > > >       "write_io_errs": "0",
> > > >       "read_io_errs": "0",
> > > >       "flush_io_errs": "0",
> > > >       "corruption_errs": "0",
> > > >       "generation_errs": "0"
> > > >     }
> > > >   ],
> > >      ^
> > > 
> > > I've verified that the comma is really there, it's not a valid json so
> > > there's a bug in the formatter. To verify that the output is valid you
> > > can use eg. 'jq', simply pipe the output of the commadn there.
> > > 
> > >   $ ./btrfs --format json dev stats /mnt | jq
> > >   parse error: Expected another key-value pair at line 16, column 1
> > 
> > I've pushed the updated plain text formatting to devel, so the only
> > remaining bug is the above extra comma.
> 
> I've tested devel branch. but there is no extra comma and I also tested
> with jq you said. jq results no error. I think that It's just mistype in
> message.

That's strange because the formatter is buggy, eg. the simplest
fmt_start()/fmt_end() leads to

$ ./json-formatter-test
{
  "__header": {
    "version": "1"
  },
}
---

Which is of course invalid and jq confirms that.

Re: [PATCH v3 2/2] btrfs-progs: device stats: add json output format

[David Sterba]

On Wed, Dec 16, 2020 at 06:23:50PM +0100, David Sterba wrote:
> On Fri, Dec 11, 2020 at 06:09:11PM +0000, Sidong Yang wrote:
> > On Fri, Dec 11, 2020 at 06:46:29PM +0100, David Sterba wrote:
> > > On Fri, Dec 11, 2020 at 06:30:25PM +0100, David Sterba wrote:
> > > > On Fri, Dec 11, 2020 at 04:48:12PM +0000, Sidong Yang wrote:
> > > > > Example json format:
> > > > > 
> > > > > {
> > > > >   "__header": {
> > > > >     "version": "1"
> > > > >   },
> > > > >   "device-stats": [
> > > > >     {
> > > > >       "device": "/dev/vdb",
> > > > >       "devid": "1",
> > > > >       "write_io_errs": "0",
> > > > >       "read_io_errs": "0",
> > > > >       "flush_io_errs": "0",
> > > > >       "corruption_errs": "0",
> > > > >       "generation_errs": "0"
> > > > >     }
> > > > >   ],
> > > >      ^
> > > > 
> > > > I've verified that the comma is really there, it's not a valid json so
> > > > there's a bug in the formatter. To verify that the output is valid you
> > > > can use eg. 'jq', simply pipe the output of the commadn there.
> > > > 
> > > >   $ ./btrfs --format json dev stats /mnt | jq
> > > >   parse error: Expected another key-value pair at line 16, column 1
> > > 
> > > I've pushed the updated plain text formatting to devel, so the only
> > > remaining bug is the above extra comma.
> > 
> > I've tested devel branch. but there is no extra comma and I also tested
> > with jq you said. jq results no error. I think that It's just mistype in
> > message.
> 
> That's strange because the formatter is buggy, eg. the simplest
> fmt_start()/fmt_end() leads to
> 
> $ ./json-formatter-test
> {
>   "__header": {
>     "version": "1"
>   },
> }
> ---
> 
> Which is of course invalid and jq confirms that.

The reason was a garbage value on the depth 0 of the stack that got
interpreted as "there were values printed, insert the comma". That also
explains why it worked for you.