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 17281
Build 17118: 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.

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

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...

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.

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

What else is preventing this from getting merged?