Page MenuHomeFreeBSD

Support destroy of multiple bookmarks on a single dataset.
Needs ReviewPublic

Authored by on Jun 10 2018, 9:22 PM.



The set of bookmarks to destroy is specified as a comma-separated list.
The syntax is the same as for snapshots, although we do not
support ranges (like with '%' for snapshots).

Along the way, support dry-run (-n), verbose (-v) and parsable (-p)
flags for the command.


  • Remove unnecessary nvlist_free (always NULL).
Test Plan

Manually tested the handling of datasets with names at ZFS_MAX_DATASET_NAME_LEN.

I have not found a way to tell Phabricator to separate out the nvlist_free churn (separate commit in my Git tree) from the main change.

Diff Detail

No Linters Available
No Unit Test Coverage
Build Status
Buildable 17154
Build 17007: arc lint + arc unit

Event Timeline retitled this revision from Summary: Support destroy of multiple bookmarks on a single dataset. to Support destroy of multiple bookmarks on a single dataset..Jun 10 2018, 9:27 PM edited the test plan for this revision. (Show Details) added reviewers: mahrens, allanjude. added a project: ZFS.

Adding range support for holds and bookmarks has been on my todo list for a while now.

I've not had time to review this yet, will try to do that temorrow.

bcr added a subscriber: bcr.Jun 11 2018, 5:26 PM

Man page nit.


You need to break the line here after the sentence stop.

mahrens requested changes to this revision.Jun 11 2018, 5:48 PM
mahrens added inline comments.

it looks like the documentation for zfs destroy snap doesn't name the subsequent snaps, so for similarity this should probably be
.Op , Ns ...


same here


Use /* for multi-line comment.


pount is not used


I don't see where this is freed in the normal (non-error) case


personally I prefer to avoid side-effects in if statement where reasonably possible. i.e.:

ds = strsep(...);
if (ds == NULL) {

you can declare n and scratch inside the loop


name can be declared inside the loop here

This revision now requires changes to proceed.Jun 11 2018, 5:48 PM marked 8 inline comments as done.Jun 12 2018, 7:19 PM

Addressed all complaints marked 'Done' in my private tree, will post a revision once @bcr responds to how I should handle the newline style vs. whitespace issue.

@mahrens Are we allowed to use __attribute__((cleanup(free)) in ZFS code?

Also, could someone point me to the style guide for the imported ZFS sources?
I assume we'll follow the upstream guide to avoid conflicts at each import...


My problem with plain zfs destroy foo#bar[,...] is that it does not make it clear that the comma-separated repetition only refers to the bookmark name bar --- it fails to communicate that zfs destroy foo#bar,foo/baz#blup is not supported (same with snapshots).

However, for uniformity, I'll follow your suggestiion.


If I break the line there, the rendered manpage contains whitespace wider than expected:


we never allocate cb.cb_nvl.

also, the local nvl in this scope is not required to be freed because we'll exit right after we return form the function...

bcr added a comment.Jun 12 2018, 7:31 PM

Thanks for the screenshot comparing the two man page renderings. I'm good with both ways.


That's a minor issue, I think. We can control how the man page is rendered only to a certain extend.
It does not look too ugly to me with the extra whitespace. You can run textproc/igor on the man page and see what it says about this issue. marked an inline comment as done.

Apply suggestions from review round 0. marked 5 inline comments as done.Jun 14 2018, 7:17 PM

Checked all 'Done' boxes, hope this is the way to use Phabricator ¯\_(ツ)_/¯

What else is preventing this from getting merged?