zdb: fix bug with -A flag

[Shreshth3]

Fixes #10544.

According to the manpage, zdb -A should ignore all assertions. But it currently does not do that. This PR fixes this bug.

How Has This Been Tested?

Ran ./zdb -K 0 tank, ./zdb -A -K 0 tank, and ./zdb -AA -K 0 tank locally.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[robn]

I'm not sure what you're seeing, but the existing code is correct in intent. -A and -AAA should downgrade assertions (ASSERT*() and VERIFY*()), while -AA and -AAA downgrades calls to zfs_panic_recover(). Your change would make -A a no-op and -AA and beyond downgrade both.

[Shreshth3]

I'm not sure what you're seeing, but the existing code is correct in intent. -A and -AAA should downgrade assertions (ASSERT*() and VERIFY*()), while -AA and -AAA downgrades calls to zfs_panic_recover(). Your change would make -A a no-op and -AA and beyond downgrade both.

Thank you for the comment, that helps me better understand the expected behavior. What I am seeing is that as we hit libspl_set_assert_ok(), if -A was set then dump_opt['A'] = 2, if -AA was set then dump_opt['A'] = 3, and if -AAA was set then dump_opt['A'] = 4. Given that this is the case, I believe the current code is incorrect. Let me know if I'm misunderstanding something.

Btw, I am running ./zdb <potentially insert -A, -AA, or -AAA> -K 0 tank to produce those results.

[robn]

Oh wow, that's actually subtly broken, and has been since -A was first introduced (illumos/illumos-gate@feef89cf5f). dump_opt['A'] is still set and advanced by dump_all and verbose, when it definitely shouldn't be. You're the first to spot it, nice one!

I'm not sure which of these is the better fix. We can just set up the assertions and recovery options before messing around with the switches:

diff --git cmd/zdb/zdb.c cmd/zdb/zdb.c
index 70a4ed46f2..c55d630673 100644
--- cmd/zdb/zdb.c
+++ cmd/zdb/zdb.c
@@ -9582,6 +9582,9 @@ main(int argc, char **argv)
 	 */
 	spa_mode_readable_spacemaps = B_TRUE;
 
+	libspl_set_assert_ok((dump_opt['A'] == 1) || (dump_opt['A'] > 2));
+	zfs_recover = (dump_opt['A'] > 1);
+
 	if (dump_all)
 		verbose = MAX(verbose, 1);
 
@@ -9592,9 +9595,6 @@ main(int argc, char **argv)
 			dump_opt[c] += verbose;
 	}
 
-	libspl_set_assert_ok((dump_opt['A'] == 1) || (dump_opt['A'] > 2));
-	zfs_recover = (dump_opt['A'] > 1);
-
 	argc -= optind;
 	argv += optind;
 	if (argc < 2 && dump_opt['R'])

Or we can explicitly exclude 'A' from special treatment by dump_all and verbose:

diff --git cmd/zdb/zdb.c cmd/zdb/zdb.c
index 70a4ed46f2..340646a34a 100644
--- cmd/zdb/zdb.c
+++ cmd/zdb/zdb.c
@@ -9586,7 +9586,9 @@ main(int argc, char **argv)
 		verbose = MAX(verbose, 1);
 
 	for (c = 0; c < 256; c++) {
-		if (dump_all && strchr("ABeEFkKlLNOPrRSXy", c) == NULL)
+		if (c == 'A')
+			continue;
+		if (dump_all && strchr("BeEFkKlLNOPrRSXy", c) == NULL)
 			dump_opt[c] = 1;
 		if (dump_opt[c])
 			dump_opt[c] += verbose;

(The real problem, perhaps, is the chaotic nature of zdb's options setup and overall structure, but that's been a thing for 20+ years, so I won't suggest that you tackle that, unless you really want to! 😅)

Both of those are untested, and there might be other options that would benefit from similar treatment. Feel free to take one or both of those patches, or do your own thing. And thanks for looking at this part of the code, its always appreciates some tender love and care!

[Shreshth3]

Thank you for the detailed comment!

Of those two options, I prefer the first one. And I actually updated the PR to include that change after sending my first comment above. Let me know if you see any issues with what I currently have.