mbox series

[0/7] btrfs-progs: implement json output for subvolume subcommands

Message ID 20230813094555.106052-1-christoph@c8h4.io (mailing list archive)
Headers show
Series btrfs-progs: implement json output for subvolume subcommands | expand

Message

Christoph Heiss Aug. 13, 2023, 9:44 a.m. UTC
JSON as output format is already implement for some of the commands in
btrfs-progs, which is a very useful feature to have for e.g. scripting.

This series adds the same for `btrfs subvolume list`, `btrfs subvolume
get-default` and `btrfs subvolume show`.

#1-#3 are some small preparatory fixes & refactors, #4 introduces the
`struct rowspec` containing all fields needed by the series.

The actual implementation of the JSON output is mostly pretty
straight-forward, only cmd_subvolume_show() needed some refactoring
first.

I probably will go about implementing the same for more commands, but
wanted to get this out now to get some feedback.

Christoph Heiss (7):
  btrfs-progs: common: document `time-long` output format
  btrfs-progs: subvol show: remove duplicated quotas error check
  btrfs-progs: subvol show: factor out text printing to own function
  btrfs-progs: subvol: introduce rowspec definition for json output
  btrfs-progs: subvol list: implement json format output
  btrfs-progs: subvol get-default: implement json format output
  btrfs-progs: subvol show: implement json format output

 cmds/subvolume-list.c  |  91 ++++++++++-
 cmds/subvolume.c       | 345 +++++++++++++++++++++++++++++------------
 cmds/subvolume.h       |  19 +++
 common/format-output.h |   2 +
 4 files changed, 357 insertions(+), 100 deletions(-)
 create mode 100644 cmds/subvolume.h

--
2.41.0

Comments

David Sterba Aug. 17, 2023, 7:34 p.m. UTC | #1
On Sun, Aug 13, 2023 at 11:44:55AM +0200, Christoph Heiss wrote:
> JSON as output format is already implement for some of the commands in
> btrfs-progs, which is a very useful feature to have for e.g. scripting.
> 
> This series adds the same for `btrfs subvolume list`, `btrfs subvolume
> get-default` and `btrfs subvolume show`.
> 
> #1-#3 are some small preparatory fixes & refactors, #4 introduces the
> `struct rowspec` containing all fields needed by the series.
> 
> The actual implementation of the JSON output is mostly pretty
> straight-forward, only cmd_subvolume_show() needed some refactoring
> first.
> 
> I probably will go about implementing the same for more commands, but
> wanted to get this out now to get some feedback.

Thanks. There are a few things regarding the json output that are still
to be figured out and to set examples to follow. The plain and json
output does not match 1:1 in the printed information, here the
'top level' does not need to be in the json output or there could be
more subvolume related info in the map. The textual output is
unfortunatelly parsed by many tools nowadays so we can't change the
format. With json it's easier to filter out the interesting data so
"more is better" in this case.

The formatter is designed in a way to allow plain text and json to be
printed by the same lines of code but this is namely for line oriented
output, like 'subvolume show'.

For the 'subvolume list' this may not be that easy so two separate
printing functions make more sense, like you did it in the series. So
that's fine.

I'll put some guidelines to the documentation, the key naming must be
unified, e.g. 'gen' or 'generation' but there's also 'transid' used in
some cases etc.

As the json format is also an ABI we need to get it finalized first so
I'll merge the series but put the actual support for --json option under
experimental build.
Christoph Heiss Aug. 18, 2023, 6:39 p.m. UTC | #2
First of, thanks for the review & merging right away!

On Thu, Aug 17, 2023 at 09:34:37PM +0200, David Sterba wrote:
>
> On Sun, Aug 13, 2023 at 11:44:55AM +0200, Christoph Heiss wrote:
> > [..]
>
> Thanks. There are a few things regarding the json output that are still
> to be figured out and to set examples to follow. The plain and json
> output does not match 1:1 in the printed information, here the
> 'top level' does not need to be in the json output or there could be
> more subvolume related info in the map.

> The textual output is unfortunatelly parsed by many tools nowadays so
> we can't change the format. With json it's easier to filter out the
> interesting data so "more is better" in this case.
Right, makes sense. I skimmed through your additional commits on top,
e.g. the null uuid thing. So all "optional" fields should rather be
`null` than missing.

>
> The formatter is designed in a way to allow plain text and json to be
> printed by the same lines of code but this is namely for line oriented
> output, like 'subvolume show'.
Yeah, I figured that after looking at it a bit more - that's why I
decided to leave most of the stuff as-is for now.

> [..]
>
> I'll put some guidelines to the documentation, the key naming must be
> unified, e.g. 'gen' or 'generation' but there's also 'transid' used in
> some cases etc.
>
> As the json format is also an ABI we need to get it finalized first
Does it make sense to explicitly document all the possible json outputs
with all their fields, i.e provide example outputs?

> so I'll merge the series but put the actual support for --json option
> under experimental build.
Thanks! Makes it easier to improve on it gradually in any case. I will
send some more patches your way rectifying these things as soon as I get
to it.
David Sterba Aug. 21, 2023, 3:19 p.m. UTC | #3
On Fri, Aug 18, 2023 at 08:39:58PM +0200, Christoph Heiss wrote:
> First of, thanks for the review & merging right away!
> On Thu, Aug 17, 2023 at 09:34:37PM +0200, David Sterba wrote:
> >
> > On Sun, Aug 13, 2023 at 11:44:55AM +0200, Christoph Heiss wrote:
> > > [..]
> >
> > Thanks. There are a few things regarding the json output that are still
> > to be figured out and to set examples to follow. The plain and json
> > output does not match 1:1 in the printed information, here the
> > 'top level' does not need to be in the json output or there could be
> > more subvolume related info in the map.
> 
> > The textual output is unfortunatelly parsed by many tools nowadays so
> > we can't change the format. With json it's easier to filter out the
> > interesting data so "more is better" in this case.
> Right, makes sense. I skimmed through your additional commits on top,
> e.g. the null uuid thing. So all "optional" fields should rather be
> `null` than missing.
> 
> >
> > The formatter is designed in a way to allow plain text and json to be
> > printed by the same lines of code but this is namely for line oriented
> > output, like 'subvolume show'.
> Yeah, I figured that after looking at it a bit more - that's why I
> decided to leave most of the stuff as-is for now.
> 
> > [..]
> >
> > I'll put some guidelines to the documentation, the key naming must be
> > unified, e.g. 'gen' or 'generation' but there's also 'transid' used in
> > some cases etc.
> >
> > As the json format is also an ABI we need to get it finalized first
> Does it make sense to explicitly document all the possible json outputs
> with all their fields, i.e provide example outputs?

Yes, examples are very useful, all of the commands supporting json
should have coverage in the tests. For startes a command that does not
change the state could just duplicate the one done in test but with
`--format=json'. Then it'll appear in the test logs and is easy to
review or copy elsewhere.

The tests/json-formatter-test.c should cover all the basic stuff, it's
also not complete so it can serve as example provider.

We may want to put the examples into the documentation then it's also
good for understanding, though this coud get out of sync over time.

> > so I'll merge the series but put the actual support for --json option
> > under experimental build.
> Thanks! Makes it easier to improve on it gradually in any case. I will
> send some more patches your way rectifying these things as soon as I get
> to it.

Thanks.