I agree that assertions are not good for the data we read from disks and that might get corrupted, but I am not sure just complaining to stderr is a good solution either. Error messages printed to stderr might be difficult to correlate to the output printed to stdout. Also since we use zdb as a pool verification tool in some CI tests, I am not sure spamming stderr without returning actual error status would be noticed.

So what would you prefer? I could print the messages to stdout instead, and eventually exit with status 1.

Exit code would definitely be good. How to better print it I am not sure though. I haven't looked close on that part of the code. But I already saw zdb using stderr for non-error messages, making it useless as error indicator. And as I have noted their correlation with stdout was a pain. So I'd say if we shoudl print the pointer, lets integrate printing of invalid values there also.

The latest commit prints all of the errors to stdout instead of stderr. It wasn't possible to print the L1 block's fill error on the same line as the block pointer itself, at least not without buffering potentially massive amounts of text in RAM, so it's printed afterwards. The output looks like this:

       a90000000   L1  DVA[0]=<5:4088dc7a0000:15000> DVA[1]=<2:3a8fade2f000:15000> [L1 ZFS plain file] fletcher4 lz4 unencrypted LE contiguous unique double size=20000L/c000P birth=7970383L/7970383P fill=16305515943967419494 cksum=000010c4104a06a3:01b38f9c940ef690:543c183d6a120c26:3f99c20b19c3eb3e
...
       a93940000    L0 DVA[0]=<14483840:7928e65e665e1a00:e200000> DVA[1]=<0:148154a00000000:1c39e00> DVA[2]=<8394880:0:1000000> [L0 unallocated] inherit inherit unencrypted BE gang unique triple size=200L/200P birth=1158310211095814315L/4409647458288664576P fill=6480302737822677281 cksum=0a4c2450925129cd:0124e955ddeccd13:0d3c3894ff64c1dd:c5d48c69c7b34245(ERROR: Block pointer type (0) does not match dnode type (19))
...
       a93a60000    L0 HOLE [L0 unallocated] size=200L birth=12694941463093612928L(ERROR: Block pointer type (0) does not match dnode type (19))
...
       a90000000: ERROR: Block pointer fill (16305515943967419494) does not match calculated value (4160680085501909892)
       a94000000   L1  DVA[0]=<0:3de2940b4000:15000> DVA[1]=<4:4174597d5000:15000> [L1 ZFS plain file] fletcher4 lz4 unencrypted LE contiguous unique double size=20000L/c000P birth=7974332L/7974332P fill=897 cksum=0000108206fb0d15:019223eac4ced4e5:0d3002f5ea989522:7045956156f0242b

It is quite verbose. I wonder what happen with some code that may try to parse the output (I think we have some in ZTS), and without error status returned still it might be difficult to handle properly.

I could change the exit status to 1 if any corruption is found. However, there is precedent for not doing that. zdb will already report corruption and yet exit 0 in the following conditions:

• unmatched FREE(s)
• Livelist ALLOC ... from TXG ... FREED at TXG ...
• DOUBLE ALLOC
• DOUBLE FREE
• Found block that crosses metaslab boundary
• Found livelist blocks marked as allocated for indirect vdevs
• while trying to open subobj id
• path not found, possibly leaked
• potentially leaked objects detected

So for consistency's sake, I think I should leave the exit status unchanged.

As discussed in the OpenZFS leadership meeting today, we agreed to change zdb to always exit non-zero if it detects any corruption. I'll create a distinct error code for that.

@amotin @allanjude with the latest commits, zdb will exit 3 if on-disk corruption was detected. It will also follow the usual convention of exiting 2 due to invalid CLI usage.

ping @amotin could you please review again?

@asomers I manually rebased this PR to get an updated CI run.