close duplicated zfs handly

[andrey-ve]

The problem described in commit 8ce0af0 Illumos

This commit handles both happy & error paths which effectively reverts
Illumos #4061.

The design to perform unconditional free in zfs_iter_dependents instead of
conditional free in iter_dependents_cb made in favor of simplicity:

• do allocation and free in the same scope
• take into account error handling execution paths inside iter_dependents_cb

Signed-off-by: Andrey Vesnovaty [email protected]

[imp]

Just want to add that this specific commit is in our internal ZFS build for almost a year. We use the library (libzfs) in a long-running application and tripped over memory leaks accumulated over time.

We felt it is time to submit the fix upstream.

[behlendorf]

@imp Thanks for posting this. It does look like a nice cleanup and simplification.

[ryao]

@ahrens Does this look good to you? If it does, could we get this into Illumos? It is a simple fix and it is nice to share patches, especially straightforward ones like this. :)

[ahrens]

It looks like you are changing the interface to zfs_iter_dependents(), such that the callback should not call zfs_close(). However, you haven't changed the callbacks, so I don't see how this works.

I agree that it this would be a preferable interface (to have the callbacks not call zfs_close(), but rather to call zfs_handle_dup() in the rare case that it's necessary). However, that change should be made to zfs_iter_*(), with corresponding changes in every callback.

[andrey-ve]

@ahrens Thanks, you absolutely right - all callbacks need to be rewritten. I'll make my best to update this pull request ASAP.

[behlendorf]

Rather than changing the interface to zfs_iter_dependents() have you investigated just fixing the leak in the existing code? While I like the changed interface better it may just be more practical to add the missing closes for now. Valgrinds --track-fds option may be helpful here.

[ahrens]

@behlendorf I agree -- fix the leak for now; it's probably a 1-line change. Then we can take our time fixing the interface.

[andrey-ve]

Considering all pros & cons I have decided to live it as is.
The fix in commit 8ce0af0 doesn't provide alloc & free in the same scope semantics taking into account the API of zfs_iter_dependents() callbacks.