Misc libzfs sendrecv cleanup #12967

ghost · 2022-01-12T19:07:07Z

Motivation and Context

I've been studying libzfs sendrecv code and cleaning up things as I go. This is far from complete, but I don't want to keep sitting on these patches.

Description

See individual commit messages for now.

How Has This Been Tested?

ZTS on FreeBSD.

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:

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.

behlendorf

It looks like most of these commits are straight forward cleanup which should be easy to get integrated. If we could for the moment drop the WIP changes from this PR we could get the cleanup in sooner. The only additional thing I'd ask is to flesh out the individual commit comments so they contain more than just a subject.

lib/libzfs/libzfs_sendrecv.c

We won't be passing a negative value here, so make it clear. Signed-off-by: Ryan Moeller <[email protected]>

De-clutter the clode and make it clear the guid is only used here. Signed-off-by: Ryan Moeller <[email protected]>

avl_add does avl_find internally, then avl_insert. We're already doing the avl_find, so using avl_insert directly avoids repeating the search. Signed-off-by: Ryan Moeller <[email protected]>

There is no need to allocate a holds nvlist. lzc_get_holds does that for us. Signed-off-by: Ryan Moeller <[email protected]>

* Add a high level comment. * Use local variables to reduce line wrapping. * Remove extra braces and insert space for clarity. * Assert precondition that the dataset name contains '@' for sanity. Signed-off-by: Ryan Moeller <[email protected]>

* Add a high level comment. * Move locals closer to point of use. * Use fnv* routines rather than explicit verification of success. * Factor out duplicated code by introducing isspacelimit to clarify behavior. Signed-off-by: Ryan Moeller <[email protected]>

* Capitalize and punctuate complete sentences in comments. * Separate out a group of locals to add a comment on their purpose. * Remove unnecessary line wrapping. * Make it clear that dds_origin is a string by using explicit character comparison to check for an empty string, rather than implictly treating it as a boolean. * Reorganize manipulation of props and holds nvlists to improve clarity. * There's no need to initialize the snapname buffer with zeros, we're immediately overwriting it. Signed-off-by: Ryan Moeller <[email protected]>

* Reduce indentation. * Move locals closer to use. Signed-off-by: Ryan Moeller <[email protected]>

* Don't bother building a debug nvlist if we can't return it. * Save errno after ioctl failure in case snprintf clobbers it. Signed-off-by: Ryan Moeller <[email protected]>

In zfs_send_progress, initialize \*bytes_written and \*blocks_visited in case we have to return early due to ioctl failure. Signed-off-by: Ryan Moeller <[email protected]>

This makes the header print before the sleep as well, which is fine. Signed-off-by: Ryan Moeller <[email protected]>

* Add missing dgettext calls. * Avoid unnecessary line wraps. * Factor out duplicated parsable check. Signed-off-by: Ryan Moeller <[email protected]>

* Add a high level comment. * Avoid unnecessary line wrapping. * Simplify size accounting logic. * Eliminate unnecessary buffer on the stack. Signed-off-by: Ryan Moeller <[email protected]>

* Add high level comments. * Eliminate unnecessarily void arg. * Avoid unnecessary line wrapping. * Initialize sdd fields with the correct types. * Remove extra whitespace. * Refactor replication checks for clarity. Signed-off-by: Ryan Moeller <[email protected]>

* Add a high level comment. * Eliminate unnecessarily void arg. * Capitalize and punctuate complete sentences in comments. Signed-off-by: Ryan Moeller <[email protected]>

* Capitalize and punctuate complete sentences. * Add a blank line between functions. Signed-off-by: Ryan Moeller <[email protected]>

Factor out get_bookmarks, find_redact_pair, and get_redact_complete helper functions to improve the readability of find_redact_book. Signed-off-by: Ryan Moeller <[email protected]>

Improve the readability of zfs_send_resume_impl by moving resume nvl decoding into a separate helper function. Signed-off-by: Ryan Moeller <[email protected]>

* Improve naming. * Reduce indentation. * Avoid boilerplate logic duplication. Signed-off-by: Ryan Moeller <[email protected]>

De-clutter the clode and make it clear the guid is only used here. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #12967

avl_add does avl_find internally, then avl_insert. We're already doing the avl_find, so using avl_insert directly avoids repeating the search. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #12967

This makes the header print before the sleep as well, which is fine. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add missing dgettext calls. * Avoid unnecessary line wraps. * Factor out duplicated parsable check. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add a high level comment. * Avoid unnecessary line wrapping. * Simplify size accounting logic. * Eliminate unnecessary buffer on the stack. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add high level comments. * Eliminate unnecessarily void arg. * Avoid unnecessary line wrapping. * Initialize sdd fields with the correct types. * Remove extra whitespace. * Refactor replication checks for clarity. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add a high level comment. * Eliminate unnecessarily void arg. * Capitalize and punctuate complete sentences in comments. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Capitalize and punctuate complete sentences. * Add a blank line between functions. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

Factor out get_bookmarks, find_redact_pair, and get_redact_complete helper functions to improve the readability of find_redact_book. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

Improve the readability of zfs_send_resume_impl by moving resume nvl decoding into a separate helper function. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Improve naming. * Reduce indentation. * Avoid boilerplate logic duplication. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

We won't be passing a negative value here, so make it clear. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

De-clutter the clode and make it clear the guid is only used here. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

avl_add does avl_find internally, then avl_insert. We're already doing the avl_find, so using avl_insert directly avoids repeating the search. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

There is no need to allocate a holds nvlist. lzc_get_holds does that for us. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add a high level comment. * Use local variables to reduce line wrapping. * Remove extra braces and insert space for clarity. * Assert precondition that the dataset name contains '@' for sanity. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Add a high level comment. * Move locals closer to point of use. * Use fnv* routines rather than explicit verification of success. * Factor out duplicated code by introducing isspacelimit to clarify behavior. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Capitalize and punctuate complete sentences in comments. * Separate out a group of locals to add a comment on their purpose. * Remove unnecessary line wrapping. * Make it clear that dds_origin is a string by using explicit character comparison to check for an empty string, rather than implictly treating it as a boolean. * Reorganize manipulation of props and holds nvlists to improve clarity. * There's no need to initialize the snapname buffer with zeros, we're immediately overwriting it. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Reduce indentation. * Move locals closer to use. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

* Don't bother building a debug nvlist if we can't return it. * Save errno after ioctl failure in case snprintf clobbers it. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967

In zfs_send_progress, initialize \*bytes_written and \*blocks_visited in case we have to return early due to ioctl failure. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#12967