Page MenuHomeFreeBSD

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

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 Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17154
Build 17007: arc lint + arc unit

Event Timeline

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.

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

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

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

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.

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.

me_cschwarz.com marked an inline comment as done.

Apply suggestions from review round 0.

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