Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libzdb 1: the libening #15804

Merged
merged 5 commits into from
Feb 5, 2024
Merged

libzdb 1: the libening #15804

merged 5 commits into from
Feb 5, 2024

Conversation

rincebrain
Copy link
Contributor

Motivation and Context

Ultimately, I'd like to break out zdb's unique functionality into something more accessible for...not-zdb, because often you might want to script zdb, or make zdb do something bespoke, and a lot of global state in one huge binary makes these kinds of changes or use cases hard.

So I'd like to start cleaning that up by ripping the giant .c file into a library and then zdb.c becomes a much smaller consumer of that library's functionality.

This is a baby step of creating a mostly empty library and statically loading it in zdb. I've got a lot more progress than this, in another branch, but my goal was more to try and see how much pushback I'd get on this so that I'm not opening a huge PR with thousands of lines of diff in dozens of commits and trying to get feedback on all of it at once.

Description

Just creates a mostly-empty libzdb and libzdb.h, and moves a few functions and defines into it.

As I said in the commit, I wouldn't suggest making any promises anytime soon about this being a stable interface - the goal is to rapidly iterate to rip chunks out, so I wouldn't suggest anyone try consuming this until the rate of churn is substantially lower, other than, obviously, zdb itself, and it's statically loaded there so you don't have some .so incompatibility breaking zdb when you needed it most...

How Has This Been Tested?

It ran. I haven't noticed any failures, and when I ran tests locally, all the failures were not, as far I saw, in zdb-related things, and were inconsistent, so I think that's just the normal ZTS flakiness.

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)
  • 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:

Step 1 in trying to slowly rip the zdb functions out of zdb.c
to allow people to play with more flexible things to leverage
zdb's functionality.

No promises on any functions or structs being stable, now or probably
in general unless someone builds a more polished abstraction, the
goal at the moment is to slowly untangle the global state usage
in zdb...

Signed-off-by: Rich Ercolani <[email protected]>
@robn
Copy link
Member

robn commented Jan 23, 2024

This is good. As a fairly regular zdb-customiser, I'm on board with the concept and your approach. I don't know if a stable libzdb API is possible or useful or desirable (I have many competing thoughts) but even if not, just a clear line between useful toolbox facilities and the frontend driver code would be a massive win.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 26, 2024
@behlendorf
Copy link
Contributor

I'm on board with this. Breaking this monolith up in to a library with a useful API creates a lot of possibility for useful tools.

Signed-off-by: Rich Ercolani <[email protected]>
@rincebrain
Copy link
Contributor Author

I'm pretty sure the test failures now are unrelated.

rpm/generic/zfs.spec.in Outdated Show resolved Hide resolved
@rincebrain
Copy link
Contributor Author

All the test failures look unrelated now, so this should be good?

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good first step to me. Can you just rebase, squash, and comment on / address the makefile feedback.

lib/libzdb/Makefile.am Outdated Show resolved Hide resolved
Signed-off-by: Rich Ercolani <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 3, 2024
@behlendorf behlendorf merged commit a0d3fe7 into openzfs:master Feb 5, 2024
24 of 25 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Step 1 in trying to slowly rip the zdb functions out of zdb.c
to allow people to play with more flexible things to leverage
zdb's functionality.

No promises on any functions or structs being stable, now or probably
in general unless someone builds a more polished abstraction, the
goal at the moment is to slowly untangle the global state usage
in zdb...

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#15804
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Step 1 in trying to slowly rip the zdb functions out of zdb.c
to allow people to play with more flexible things to leverage
zdb's functionality.

No promises on any functions or structs being stable, now or probably
in general unless someone builds a more polished abstraction, the
goal at the moment is to slowly untangle the global state usage
in zdb...

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#15804
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Step 1 in trying to slowly rip the zdb functions out of zdb.c
to allow people to play with more flexible things to leverage
zdb's functionality.

No promises on any functions or structs being stable, now or probably
in general unless someone builds a more polished abstraction, the
goal at the moment is to slowly untangle the global state usage
in zdb...

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes openzfs#15804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants