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

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

Details

Reviewers

mahrens
allanjude

Summary

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.

Churn:

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

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 17154
Build 17007: arc lint + arc unit

Event Timeline

me_cschwarz.com created this revision.Jun 10 2018, 9:22 PM

Herald added a subscriber: delphij. · View Herald TranscriptJun 10 2018, 9:22 PM

Harbormaster completed remote builds in B17154: Diff 43571.Jun 10 2018, 9:22 PM

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

me_cschwarz.com edited the test plan for this revision. (Show Details)

me_cschwarz.com added reviewers: mahrens, allanjude.

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

cddl/contrib/opensolaris/cmd/zfs/zfs.8
1896	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.

cddl/contrib/opensolaris/cmd/zfs/zfs.8
72	it looks like the documentation for `zfs destroy snap` doesn't name the subsequent snaps, so for similarity this should probably be `.Op , Ns ...`
1884	same here
cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
1238–1239	Use `/*` for multi-line comment.
1243	`pount` is not used
1247	I don't see where this is freed in the normal (non-error) case
1253	personally I prefer to avoid side-effects in `if` statement where reasonably possible. i.e.: ds = strsep(...); if (ds == NULL) {
1260	you can declare `n` and `scratch` inside the loop
1452	`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...

cddl/contrib/opensolaris/cmd/zfs/zfs.8
72	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.
1896	If I break the line there, the rendered manpage contains whitespace wider than expected:
cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
1426	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.

cddl/contrib/opensolaris/cmd/zfs/zfs.8
1896	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.