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

Man page nit.

cddl/contrib/opensolaris/cmd/zfs/zfs.8
1895

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
71

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

1883

same here

cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
1237–1238

Use /* for multi-line comment.

1242

pount is not used

1246

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

1252

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

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

you can declare n and scratch inside the loop

1456

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
71

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.

1895

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
1895

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.

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

What else is preventing this from getting merged?