geom: more libxo fixes

[guest-jsollvander]

Summary

After https://reviews.freebsd.org/D37615 was merged I received a some feedback on how to better use libxo from Phil, which can be found in https://reviews.freebsd.org/D53110. This PR addresses some of it, like tags should be lowercase, but it does not remove all nested containers/lists and replace them with {k:} or {l:} where it should be possible, as I thought that made the JSON/XML output look wrong from how I wanted it to look. I also renamed a few containers so that the JSON/XML output says "DISK" or "MULTIPATH" (depending on class) instead of a generic "Geom", which I thought was a nice touch, and it makes the --libxo:XP output resemble kern.geom.confxml some more.

Added some conditional checks to handle null derefs that appeared on some places.

As I was working on the above I noticed that the previous patches "broke" geom <class> status -s (script mode), the normal output was looking slightly off from how it used to, so I fixed that. And I also noticed that the JSON/XML outputted data looked different from when you used the flag vs if you didn't. This is now also fixed. Lastly, I added {t:} to trim extra whitespaces that sometimes appeared in the value fields of JSON/XML output.

Test Plan

Output looks good
No tests fail

[mckusick]

Structurally, this looks good to me. I will await Phil's commentary on its JSON/XML usefulness.

[phil]

sbin/geom/core/geom.c

903

Use "-" to separate words like "media-size" and "sector-size".

1208

Can you give me some details on what's going on here? What are you wanting the text output to look like? Are you just wanting the titles on the first item (outside the list)?

Where does "name" come from and it is safe from user input/injection?

[guest-jsollvander]

sbin/geom/core/geom.c

903

Is that a wise thing to do? It's called "mediasize" in kern.geom.confxml and so on. I'm thinking the output from geom --libxo:XP <class> list/status and sysctl kern.geom.confxml should look more or less the same. But I can of course change it if you have strong opinion about it.

1208

I'm going to start with the second question first: Name is the name of the geom (disk, partition, multipath label, geli label ... ) taken from ggeom->lg_name (ggeom is defined in libgeom) , basically the first field when you run gpart status or gmultipath status for example. Is it safe? I think so, if you supply a name as input to status it needs to be validated through gctl_get_ascii first.

Sure! I'll show you and give you some commands if you want to try it yourself as well:

root@johan_test:~ # mdconfig -t swap -s 1M
md0
root@johan_test:~ # mdconfig -t swap -s 1M
md1
root@johan_test:~ # mdconfig -t swap -s 1M
md2
root@johan_test:~ # mdconfig -t swap -s 1M
md3
root@johan_test:/usr/src # gmultipath create -A test1 /dev/md0 /dev/md1
root@johan_test:/usr/src # gmultipath create -A test1 /dev/md2 /dev/md3
root@johan_test:/usr/src # gmultipath status
           Name   Status  Components
multipath/test1  OPTIMAL  md0 (ACTIVE)
                          md1 (ACTIVE)
multipath/test2  OPTIMAL  md2 (ACTIVE)
                          md3 (ACTIVE)

root@johan_test:/usr/src # gmultipath status -s
multipath/test1  OPTIMAL  md0 (ACTIVE)
multipath/test1  OPTIMAL  md1 (ACTIVE)
multipath/test2  OPTIMAL  md2 (ACTIVE)
multipath/test2  OPTIMAL  md3 (ACTIVE)

That is how the normal output looks. Under normal runs it prints the name, status and the component's name and state on the first iteration and blanks name and status on the remaining iterations, leaving only the component's name and state. The -s flag is supposed to give you a script friendly output and doesn't blank those lines as a result. IMO the -s flag should be deprecated in favor of using --libxo:.. some day.

Anyways, here it is in JSON format:

root@johan_test:/usr/src # gmultipath --libxo:JP status
{
  "__version": "1",
  "MULTIPATH": [
    {
      "name": "multipath/test1",
      "status": "OPTIMAL",
      "components": [
        {
          "component": "md0",
          "state": "ACTIVE"
        },
        {
          "component": "md1",
          "state": "ACTIVE"
        }
      ]
    },
    {
      "name": "multipath/test2",
      "status": "OPTIMAL",
      "components": [
        {
          "component": "md2",
          "state": "ACTIVE"
        },
        {
          "component": "md3",
          "state": "ACTIVE"
        }
      ]
    }
  ]
}

We open a JSON array called "components" and create a "component" JSON object containing the component name and state (and synchronization state when applicable) and add it to the array. This is the format I think most would expect here. But, without this patch and while using the -s you get this instead:

root@johan_test:/usr/src # gmultipath --libxo:JP status -s
{
  "__version": "1",
  "MULTIPATH": [
    {
      "name": "multipath/test1",
      "status": "OPTIMAL",
      "components": [
        {
          "component": "md0",
          "state": "ACTIVE"
        },
      ],
      "components": [
        {
          "component": "md1",
          "state": "ACTIVE"
        }
      ]
    },
    {
      "name": "multipath/test2",
      "status": "OPTIMAL",
      "components": [
        {
          "component": "md2",
          "state": "ACTIVE"
        },
       ],
       "components": [
        {
          "component": "md3",
          "state": "ACTIVE"
        }
      ]
    }
  ]
}

The JSON array "components" is opened, the "component" JSON object gets added to it, but on the second iteration (and third, and forth) a new "components" JSON array is opened and the newest "component" JSON object gets added to that one. I tried multiple field modifiers and field roles to try and fix this (not sure if I tried {T:} though), I also tried setting markers, but that only caused the JSON object to have a JSON array inside of it.

[brd]

This looks good to me. Did you run the tests?

[guest-jsollvander]

In D53313#1219500, @brd wrote:

This looks good to me. Did you run the tests?

I only ran the testsuite in /usr/tests/sys/geom/class and all of those tests came back OK.

[brd]

In D53313#1219992, @guest-jsollvander wrote:

In D53313#1219500, @brd wrote:

This looks good to me. Did you run the tests?

I only ran the testsuite in /usr/tests/sys/geom/class and all of those tests came back OK.

Great, thanks!

[mckusick]

Unless anyone has further feedback, I will plan to commit this early next week.