LGTM, but you should also create a real Phabricator account now.

@jrtc27's suggestion works. Though there are other problems too, that I haven't solved yet.

• Respond to Jessica's feedback.

LGTM.

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

In D53950#1235311, @guest-jsollvander wrote:

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

Thanks Alan!

Could you explain why "type" is wrong here? According to the manpage for gpart show this field describes the "... the partition type, ...", so I thought "type" would be suitable field name for "element"? I'll still change it, I'm just curious.

Easy enough to fix.

Let me see what I can do about the attribute field.

Nice job! I suggest a few changes, though.

My 13th gen Framework 13 laptop works ok with or without this patch.

Closing in favor of https://reviews.freebsd.org/D52780

You must also update the test cases in tests/sys/fs/fusefs/fallocate.cc . In particular, I think that the PosixFallocate.eopnotsupp will fail now, unless you update it.

LGTM. Thanks for the contribution, Juraj.

Thanks for doing this. I think it will be a good addition. But I'm curious: why did you choose -F? Obviously -u and -U were already taken.

In D53080#1217336, @guest-svmhdvn wrote:

@asomers As of f4f638eb23d770e19ede167908d8145b8851f835, this test is still failing intermittently in CI as seen here on aarch64: https://ci.freebsd.org/view/Test/job/FreeBSD-main-aarch64-test/1801/testReport/junit/sys.fs.fusefs/bad_server/main.

This seems to also be reproducible locally on amd64 with more than 20+ minutes of testing (very intermittent, but becomes clear when running on slower platforms). I'm not sure this fix is comprehensive, although it did certainly help reduce the intermittency.

Thanks for getting this fixed, @arrowd .

@arrowd now that you've committed the main bmap patch, are you ok with this test?

That explains it. I was never testing with SCTP, and I doubt that Damin was, either.

Can you give an example of the incorrect output? Does it matter whether "-q" is in use?

LGTM

Looks like a good start.

You need to wrap that long line to 80 cols, but otherwise it LGTM.

I have two comments in addition to the inline ones:

In D53079#1212648, @glebius wrote:

I trust Alan's expertise here. IMHO, given that this patch is analog to fusefs one, should be fine. But not expert in the area.

I too am confused by the commit message. Won't the column always be shown in json and xml output, if "-s" is used?

Could you please give an example of the before and after output?

Could you give an example of what the output looks like before and after?

In D52914#1209926, @olivier wrote:

In D52914#1209913, @asomers wrote:

The MD_LEN variable is now badly named. I think it's ok to leave its value the same. Most test cases don't require 1 MB of I/O. But you should at least change that name.

So, let’s use DEVICE_IO_LEN then (to clarifies that this constant represents the I/O buffer length used for device tests (16 KB), not the size of the MD device itself which is now 1 MB) ?

The MD_LEN variable is now badly named. I think it's ok to leave its value the same. Most test cases don't require 1 MB of I/O. But you should at least change that name.

After seeing this code in practice it seems quite complicated. I do think that the kernel based approach would be simpler. But an important question is: what file systems already use this option on Linux? How many of them would work with either this implementation, or the hypothetical kernel-based one?

So is -1 being cast to true? Thank you GCC for telling us that. If so, I think we should replace ATF_REQUIRE_MSG with ATF_REQUIRE_EQ_MSG. I think that would be more clear.

What's the problem? Under what conditions is the current code incorrect?