Fix failing multipath test caused by D37615

[js]

Summary

D37615 caused several gmultipath tests to fail. The tests in question expected a pattern that was missing from the output of geom <class> list, this patch fixes it.

root@johan_test:/usr/tests/sys/geom/class/multipath # kyua report
===> Skipped tests
failloop:failloop  ->  skipped: Required configuration property 'allow_sysctl_side_effects' not defined  [0.002s]
===> Summary
Results read from /root/.kyua/store/results.usr_tests_sys_geom_class_multipath.20251015-085403-959737.db
Test cases: 12 total, 1 skipped, 0 expected failures, 0 broken, 0 failed
Total time: 1.003s

Test Plan

Fix failing tests.

[imp]

sbin/geom/core/geom.c

971

Is the json and xml still right?

[js]

Yes. I took a json sample before and after applying the patch locally and it looks ok. No extra ".".

Before:

geom disk list --libxo json,pretty
{
  "__version": "1",
  "Geoms": [
    {
      "Name": "vtbd0",
      "Providers": [
        {
          "Name": "vtbd0",
          "Mediasize": 21474836480,
          "Sectorsize": 512,
          "descr": "(null)",
          "ident": "BHYVE-22E5-A7A0-F77F",
          "rotationrate": "unknown",
          "fwsectors": "0",
          "fwheads": "0"
        }
      ]
    }
  ]
}

after:

geom disk list --libxo json,pretty
{
  "__version": "1",
  "Geoms": [
    {
      "Name": "vtbd0",
      "Providers": [
        {
          "Name": "vtbd0",
          "Mediasize": 21474836480,
          "Sectorsize": 512,
          "descr": "(null)",
          "ident": "BHYVE-22E5-A7A0-F77F",
          "rotationrate": "unknown",
          "fwsectors": "0",
          "fwheads": "0"
        }
      ]
    }
  ]
}

[jlduran]

Would it be worth addressing these suggestions here as well?:
https://lists.freebsd.org/archives/dev-commits-src-main/2025-October/036657.html

[js]

In D53110#1213418, @jlduran wrote:

Would it be worth addressing these suggestions here as well?:
https://lists.freebsd.org/archives/dev-commits-src-main/2025-October/036657.html

Thanks for linking that, I hadn't seen it. And yes; I'll start working on that, that is some good feedback.

If someone wants to merge this right away to fix the tests I can open a new review for that, or we just revert 0d4642a67e59 and I make a new PR with everything included. Either way is fine with me.

[mckusick]

I will put in this change to fix the tests.
Pleas start a new review to address the additional suggestions.

[phil]

[Here are the review comments I sent Kirk]

On 13 Oct 2025, at 17:16, Kirk McKusick wrote:
>  static void
> -list_one_provider(struct gprovider *pp, const char *prefix)
> +list_one_provider(struct gprovider *pp, const char *padding)
>  {
>         struct gconfig *conf;
>         char buf[5];
>
> -       printf("Name: %s\n", pp->lg_name);
> +       xo_emit("{Lcw:Name}{:Name}\n", pp->lg_name);
>         humanize_number(buf, sizeof(buf), (int64_t)pp->lg_mediasize, ""=
,
>             HN_AUTOSCALE, HN_B | HN_NOSPACE | HN_DECIMAL);

Tag names should be lower case, for consistency and ease of use: s/:Name/=
:name/
(This applies throughout)

> -       printf("%sMediasize: %jd (%s)\n", prefix, (intmax_t)pp->lg_medi=
asize,
> -           buf);
> -       printf("%sSectorsize: %u\n", prefix, pp->lg_sectorsize);
> +       xo_emit("{P:/%s}{Lcw:Mediasize}{:Mediasize/%jd} ({N:/%s})\n",
> +           padding, (intmax_t)pp->lg_mediasize, buf);
> +       xo_emit("{P:/%s}{Lcw:Sectorsize}{:Sectorsize/%u} \n",
> +           padding, pp->lg_sectorsize);

Using the "humanize" features will allow human-readable numbers in human-=
friendly styles (text, html) and real numbers in encoding styles (xml and=
 json).

https://libxo.readthedocs.io/en/latest/field-modifiers.html?highlight=3Dh=
umanize#field-modifiers-1

		Modifiers
		~~~~~~~~~~~~~~~

		Field modifiers are flags which modify the way content emitted for
		particular output styles:

		  ======================
		   M   Name            Description
		  ======================
		...
		   h   humanize (hn)   Format large numbers in human-readable style
		  \    hn-space        Humanize: Place space between numeric and unit
		  \    hn-decimal      Humanize: Add a decimal digit, if number < 10
		  \    hn-1000         Humanize: Use 1000 as divisor instead of 1024

Hmm.... looks like I'm lacking a flag for HN_B, sadly.  Not sure how I mi=
ssed that.  I'll open a PR to add it.

If you want the full numbers, you can use the "e" flag to restrict emitti=
ng values to encodings styles.

>         if (pp->lg_stripesize > 0 || pp->lg_stripeoffset > 0) {
> -               printf("%sStripesize: %ju\n", prefix, pp->lg_stripesize=
);
> -               printf("%sStripeoffset: %ju\n", prefix, pp->lg_stripeof=
fset);
> +               xo_emit("{P:/%s}{Lcw:Stripesize}{Stripesize/%ju}\n",
> +                   padding, pp->lg_stripesize);
> +               xo_emit("{P:/%s}{Lcw:Stripeoffset}{Stripeoffset/%ju}\n"=
,
> +                   padding, pp->lg_stripeoffset);

Missing some colons before the tag names.  Testing with the "warn" flag s=
hould point this out, if the code gets hit.

Style-wise, using hyphens ("{:strip-size/%ju}", "{:strip-offset/%ju}") he=
lps make readable tag names.

> -       printf("%sMode: %s\n", prefix, pp->lg_mode);
> +       xo_emit("{P:/%s}{Lcw:Mode}{Mode}\n", padding, pp->lg_mode);

{:mode}

> @@ -940,27 +958,36 @@ list_one_geom(struct ggeom *gp)
>         struct gconfig *conf;
>         unsigned n;
>
> -       printf("Geom name: %s\n", gp->lg_name);
> +       xo_emit("{Lcw:Geom name}{:Name}\n", gp->lg_name);
>         LIST_FOREACH(conf, &gp->lg_config, lg_config) {
> -               printf("%s: %s\n", conf->lg_name, conf->lg_val);
> +               xo_emit("{Lcwa:}{a:}\n", conf->lg_name, conf->lg_name,
> +                   conf->lg_val);
>         }
>         if (!LIST_EMPTY(&gp->lg_provider)) {
> -               printf("Providers:\n");
> +               xo_open_list("Providers");

Style-wise, list and container names should also be lower case.

> +               xo_emit("{Tc:Providers}\n");
>                 n =3D 1;
>                 LIST_FOREACH(pp, &gp->lg_provider, lg_provider) {
> -                       printf("%u. ", n++);
> +                       xo_emit("{T:/%u} ", n++);
> +                       xo_open_instance("provider");
>                         list_one_provider(pp, "   ");
> +                       xo_close_instance("provider");
>                 }
> +               xo_close_list("Providers");
>         }
>         if (!LIST_EMPTY(&gp->lg_consumer)) {
> -               printf("Consumers:\n");
> +               xo_open_list("Consumers");
> +               xo_emit("{Tc:Consumers}\n");
>                 n =3D 1;
>                 LIST_FOREACH(cp, &gp->lg_consumer, lg_consumer) {
> -                       printf("%u. ", n++);
> +                       xo_emit("{T:/%u} ", n++);
> +                       xo_open_instance("consumer");
>                         list_one_consumer(cp, "   ");
> +                       xo_close_instance("consumer");
>                 }
> +               xo_close_list("Consumers");
>         }

For simple lists like this, use the "l" flag here, since it will yield mo=
re useable JSON (dropping the open/close list/instance):

https://libxo.readthedocs.io/en/latest/field-modifiers.html?highlight=3Dl=
eaf#the-leaf-list-modifier-l

		The Leaf-List Modifier ({l:})
		+++++++++++++++++++++++++++++

		.. index:: Field Modifiers; Leaf-List

		The leaf-list modifier is used to distinguish lists where each
		instance consists of only a single value.  In XML, these are
		rendered as single elements, where JSON renders them as arrays::

		    EXAMPLE:
		        for (i =3D 0; i < num_users; i++) {
		            xo_emit("Member {l:user}\\n", user[i].u_name);
		        }
		    XML:
		        <user>phil</user>
		        <user>pallavi</user>
		    JSON:
		        "user": [ "phil", "pallavi" ]

		The name of the field must match the name of the leaf list.

FWIW, there's some example code showing the differences between list and =
leaf-list rendering in XML and JSON appended below.

> @@ -1038,14 +1067,20 @@ std_list(struct gctl_req *req, unsigned flags _=
_unused)
>                                     "an instance named '%s'.",
>                                     gclass_name, name);
>                         }
> +                       xo_open_container("Geom");
>                         list_one_geom(gp);
> +                       xo_close_container("Geom");
>                 }
>         } else {
> +               xo_open_list("Geoms");
>                 LIST_FOREACH(gp, &classp->lg_geom, lg_geom) {
>                         if (LIST_EMPTY(&gp->lg_provider) && !all)
>                                 continue;
> +                       xo_open_instance("geom");
>                         list_one_geom(gp);
> +                       xo_close_instance("geom");
>                 }
> +               xo_close_list("Geoms");

Same here ("l:").

> +       xo_open_instance("status");
>         LIST_FOREACH(cp, &gp->lg_consumer, lg_consumer) {
> -               component =3D status_one_consumer(cp);
> -               if (component =3D=3D NULL)
> +               cstate =3D status_one_consumer(cp, "state");
> +               csyncr =3D status_one_consumer(cp, "synchronized");
> +               if (cstate =3D=3D NULL && csyncr =3D=3D NULL)
>                         continue;
> +               if (!gotone || script) {
> +                       if (!gotone) {
> +                               xo_emit("{:name/%*s}  {:status/%*s}  ",=

> +                                   name_len, name, status_len, status)=
;
> +                       } else {
> +                               xo_emit("{d:name/%*s}  {d:status/%*s}  =
",
> +                                   name_len, name, status_len, status)=
;
> +                       }
> +                       xo_open_list("components");
> +               }
> +
> +               xo_open_instance("components");
> +               if (cstate !=3D NULL && csyncr !=3D NULL) {
> +                       xo_emit("{P:/%*s}{:compontent} ({:state}, {:syn=
chronized})\n",
> +                       len, "", cp->lg_provider->lg_name, cstate, csyn=
cr);
> +               } else if (cstate !=3D NULL) {
> +                       xo_emit("{P:/%*s}{:compontent} ({:state})\n",
> +                       len, "", cp->lg_provider->lg_name, cstate);
> +               } else {
> +                       xo_emit("{P:/%*s}{:compontent} ({:synchronized}=
)\n",
> +                       len, "", cp->lg_provider->lg_name, csyncr);
> +               }

Extra "n"s in the tag names: s/compontent/component/

> +               xo_close_instance("components");

Do these instances have keys?  Or is this a simple list (leaf-list)?

>                 gotone =3D 1;
> -               printf("%*s  %*s  %s\n", name_len, name, status_len, st=
atus,
> -                   component);
> -               if (!script)
> -                       name =3D status =3D "";
> +               if (!len && !script)
> +                       len =3D name_len + status_len + 4;
>         }
>         if (!gotone) {
> -               printf("%*s  %*s  %s\n", name_len, name, status_len, st=
atus,
> -                   "N/A");
> +               xo_emit("{:name/%*s}  {:status/%*s}  ", name_len, name,=
 status_len, status);
> +               xo_open_list("components");
> +               xo_open_instance("components");
> +               xo_emit("{P:/%*s}{d:compontent}\n", len, "", "N/A");
> +               xo_close_instance("components");
>         }
> +       xo_close_list("components");
> +       xo_close_instance("status");

Avoid the close_list call if you didn't call open_list.  libxo's internal=
 state transition FSM will generally catch this (and warn you), but it's =
better to avoid it.

Otherwise looks good.

Thanks,
 Phil

-----------------

Bock % cat /tmp/foo.c
#include <libxo/xo.h>

const char *user[] =3D { "alice", "bob", "carl", "doug", NULL };
const int num_users =3D 4;

int
main (int argc, char **argv)
{
    int i;

    argc =3D xo_parse_args(argc, argv);
    if (argc < 0)
        return (argc);

    int yes =3D (argc > 1);

    xo_open_container("top");

    if (yes) {
        for (i =3D 0; i < num_users; i++) {
            xo_emit("Member {l:user}\n", user[i]);
        }
    } else {
        xo_open_list("test");

        for (i =3D 0; i < num_users; i++) {
            xo_open_instance("test");
            xo_emit("Member {k:user}\n", user[i]);
            xo_close_instance("test");
        }
        xo_close_list("test");
    }

    xo_close_container("top");

    xo_finish();
    exit(0);
}
Bock % cc -I work/root/include/ -L work/root/lib -lxo -o /tmp/foo /tmp/fo=
o.c
Bock % /tmp/foo
Member alice
Member bob
Member carl
Member doug
Bock % /tmp/foo --libxo:XP
<top>
  <test>
    <user>alice</user>
  </test>
  <test>
    <user>bob</user>
  </test>
  <test>
    <user>carl</user>
  </test>
  <test>
    <user>doug</user>
  </test>
</top>
Bock % /tmp/foo --libxo:XP yes
<top>
  <user>alice</user>
  <user>bob</user>
  <user>carl</user>
  <user>doug</user>
</top>
Bock % /tmp/foo --libxo:JP
{
  "top": {
    "test": [
      {
        "user": "alice"
      },
      {
        "user": "bob"
      },
      {
        "user": "carl"
      },
      {
        "user": "doug"
      }
    ]
  }
}
Bock % /tmp/foo --libxo:JP yes
{
  "top": {
    "user": [
      "alice",
      "bob",
      "carl",
      "doug"
    ]
  }
}

[phil]

In D53110#1213218, @guest-jsollvander wrote:

Yes. I took a json sample before and after applying the patch locally and it looks ok. No extra ".".

I don't see extra quotes; where were you seeing them?

geom disk list --libxo json,pretty

The "--libxo" options should be processed first, and should appear first on the command line, not intermixed with other options, like:

geom --libxo:JP disk list

"descr": "(null)",

Has this null deref been addressed?

Thanks,
Phil

[js]

In D53110#1216699, @phil wrote:

In D53110#1213218, @guest-jsollvander wrote:

Yes. I took a json sample before and after applying the patch locally and it looks ok. No extra ".".

I don't see extra quotes; where were you seeing them?

geom disk list --libxo json,pretty

The "--libxo" options should be processed first, and should appear first on the command line, not intermixed with other options, like:

geom --libxo:JP disk list

"descr": "(null)",

Has this null deref been addressed?

Thanks,
Phil

The JSON/XML output was fine, no extra quotes or other characters. The thing that was missing was a . in the normal output because I accidentally forgot to add it when I rewrote geom.c to use libxo. Here is how it was supposed to look like:

root@slc-rb3-ss:~ # geom disk list |head
Geom name: da75
Providers:
1. Name: da75
   Mediasize: 8001563222016 (7.3T)
   Sectorsize: 4096
   Mode: r2w2e2
   descr: SEAGATE ST8000NM0065
   lunid: 5000c50094744a7f
   ident: ZA18RNMW0000R804L4L5
   rotationrate: 7200

See that the title starts with 1. Name: ... ? Before this patch it said 1 Name: ... which broke the multipath tests.

That null ref deref has not been addressed yet, I don't think I even noticed it before you pointed it out. This is how the disk looks like in kern.geom.confxml:

<class id="0xffffffff81ab4e68">
    <name>DISK</name>
    <geom id="0xfffff80003421800">
      <class ref="0xffffffff81ab4e68"/>
      <name>vtbd0</name>
      <rank>1</rank>
      <config>
      </config>
        <provider id="0xfffff80003421700">
          <geom ref="0xfffff80003421800"/>
          <mode>r3w3e6</mode>
          <name>vtbd0</name>
          <mediasize>21474836480</mediasize>
          <sectorsize>512</sectorsize>
          <stripesize>131072</stripesize>
          <stripeoffset>0</stripeoffset>
          <config>
            <fwheads>0</fwheads>
            <fwsectors>0</fwsectors>
            <rotationrate>unknown</rotationrate>
            <ident>BHYVE-22E5-A7A0-F77F</ident>
            <descr></descr>
          </config>
        </provider>
    </geom>
  </class>

I can add a conditional check for conf->lg_val == NULL in list_one_provider()/list_one_consumer() in the patch I'm working on right now. With some luck I should be able to post it later this week.

[js]

There we go:

root@johan_test:/usr/src # geom --libxo:JP disk list
{
  "__version": "1",
  "DISK": [
    {
      "name": "vtbd0",
      "providers": [
        {
          "name": "vtbd0",
          "mediasize": 21474836480,
          "sectorsize": 512,
          "stripe-size": 131072,
          "stripe-offset": 0,
          "mode": "r3w3e6",
          "descr": "",
          "ident": "BHYVE-22E5-A7A0-F77F",
          "rotationrate": "unknown",
          "fwsectors": "0",
          "fwheads": "0"
        }
      ]
    }
  ]
}

But as I said, I have a few more things I'd like to address before posting a patch.