Re-uploaded all my changes.

In D53950#1235417, @asomers wrote:

Nice! It almost LGTM , but I agree with @ziaee that you should remove the man page link to libxo(3).

This should address all the things. Here is the new sample output:

In D53950#1235136, @asomers wrote:

Nice job! I suggest a few changes, though.

• I suggest using the same field names that gpart list does, where possible.
  • logical_starting_block => start
  • size_in_blocks => sectors
  • provider_name => name
  • attribute => attrib (or better yet, change "gpart list" to show "attribute" instead of "attrib")
• When you do "gpart show -l --libxo=json", it will wrongly print something like "type": "swap0". That must be fixed. It should either show "label": "swap0", or just always have separate fields for "label" and "type".
• There's an asymmetry in the way that "gpart show --libxo=json" displays gaps in the middle of the partition table vs at the end. It only prints "free" if they're in the middle, and they're only in the "unallocated" section if they're at the end. I like the first way better. I think you should get rid of the "unallocated" section.
• Also, the json formatting for "attribute" isn't very machine-parseable. Right now it shows " [bootonce,bootme] ". I'm not sure what format would be best.
• Don't forget to bump the .Dd dates in the man pages

In D53313#1231715, @markj wrote:

In D53313#1230777, @guest-jsollvander wrote:

In D53313#1230601, @markj wrote:

In D53313#1230600, @markj wrote:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=290629 reports a bug in the libxoification of "gpart show":

> gpart show --libxo=json,pretty
=>        34  3907029101  nda0  GPT  (1.8T)
          34           6        - free -  (3.0K)
          40      532480     1  efi  (260M)
      532520        1024     2  freebsd-boot  (512K)
      533544         984        - free -  (492K)
      534528    67108864     3  freebsd-swap  (32G)
    67643392  3839385600     4  freebsd-zfs  (1.8T)
  3907028992         143        - free -  (72K)

=>        40  3907029088  nda1  GPT  (1.8T)
          40      532480     1  efi  (260M)
      532520        1024     2  freebsd-boot  (512K)
      533544         984        - free -  (492K)
      534528    67108864     3  freebsd-swap  (32G)
    67643392  3839385600     4  freebsd-zfs  (1.8T)
  3907028992         136        - free -  (68K)

{ }

@guest-jsollvander would you be able to look into this?

Hmm, on second look, it seems that only "list" and "status" subcommands are supported? In that case, we should return an error if --libxo is specified.

Correct, I've only added libxo support for list and status subcommands. I can look into adding support for the rest starting next week as it shouldn't be too hard. But I can't provide you with a timeline when I'll get it done unfortunately, so if we want to close this bug fast then we should simply return an error like you suggested.

In the meantime, would you submit a patch which turns that case into an error?

In D53313#1230601, @markj wrote:

In D53313#1230600, @markj wrote:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=290629 reports a bug in the libxoification of "gpart show":

> gpart show --libxo=json,pretty
=>        34  3907029101  nda0  GPT  (1.8T)
          34           6        - free -  (3.0K)
          40      532480     1  efi  (260M)
      532520        1024     2  freebsd-boot  (512K)
      533544         984        - free -  (492K)
      534528    67108864     3  freebsd-swap  (32G)
    67643392  3839385600     4  freebsd-zfs  (1.8T)
  3907028992         143        - free -  (72K)

=>        40  3907029088  nda1  GPT  (1.8T)
          40      532480     1  efi  (260M)
      532520        1024     2  freebsd-boot  (512K)
      533544         984        - free -  (492K)
      534528    67108864     3  freebsd-swap  (32G)
    67643392  3839385600     4  freebsd-zfs  (1.8T)
  3907028992         136        - free -  (68K)

{ }

@guest-jsollvander would you be able to look into this?

Hmm, on second look, it seems that only "list" and "status" subcommands are supported? In that case, we should return an error if --libxo is specified.

In D53313#1219500, @brd wrote:

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

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.

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

We should add the same check to status_one_geom() as well

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

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

In D37615#1213052, @mckusick wrote:

In D37615#1212734, @guest-jsollvander wrote:

In D37615#1212613, @guest-svmhdvn wrote:

This patch seems to break several testcases in the sys/geom/class/multipath/misc test suite with this output:

===> sys/geom/class/multipath/misc:add
Result:     failed: ACTIVE !=  (ACTIVE != )

I'll take a look.

If you can come up with a fix in the next 24 hours, I will wait to apply it. If you cannot do it in that time frame, let me know, and I will revert the commit until you can sort it out.

In D37615#1212613, @guest-svmhdvn wrote:

This patch seems to break several testcases in the sys/geom/class/multipath/misc test suite with this output:

===> sys/geom/class/multipath/misc:add
Result:     failed: ACTIVE !=  (ACTIVE != )

AFAIK this is still a problem

Sorry to bother people, but could we revive this review? I was recently talking to some people who noticed this the hard way and it would be great to get something merged that fixes it, even if it isn't perfect. I think this looks good enough and is definitely not "the ugliest kludge to make this work"

I see that bootconfig used to have code for adding ESPs to all ZFS/UFS boot disks, but it was removed in https://reviews.freebsd.org/D28897. @imp reviewed that one, maybe you could share your opinion here if we should bring parts of that code back? I think all disks that are configured as boot disks by the installer should a working ESP after installation. If the wrong boot disks die then you are left with a system without any ESPs at all.

Bump

Updated diff to include context

So I experimented a little and remade this as "geom -c <consumer>". But unless I figure out a good way to filter by class it is useless. Right now it will just pick whatever class the provider first appears in, like DEV. So the proper way, IMO, to do that would be "geom <class> which <consumer>" and not just add another flag. But then "which" would only exist when using "geom" directly and not when using "gmultipath"/"gpart"/"glabel" binaries which I think would look weird. What are your thoughts?

In D37309#847728, @mav wrote:

Not that I have specific objections against this, but I think it would be much better to implement a generic way to print all consumers connected to specified GEOM provider. This code does not look anyhow specific to gmultipath.