Page MenuHomeFreeBSD

Extend 'zfs holds -r' to accept a dataset, instead of only a snapshot as input
ClosedPublic

Authored by allanjude on Oct 23 2015, 8:46 PM.

Details

Test Plan

zfs holds dataset@snapshot
zfs holds -r dataset@snapshot
zfs holds -r dataset

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude updated this revision to Diff 9670.Oct 23 2015, 8:46 PM
allanjude retitled this revision from to Extend 'zfs holds -r' to accept a dataset, instead of only a snapshot as input.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added a reviewer: dteske.
dteske accepted this revision.Oct 23 2015, 8:54 PM
dteske edited edge metadata.

Nice work. I gauge this on style(9) rules (NULL used properly; '\0' used properly; proper comparison of pointer to NULL versus treating it as boolean; et cetera), validation of logic-reversal (== becomes !=), validation of removal of error message given context of change (allow dataset arguments as well as snapshots), and of course this change meets the goal of minimal-changes to reach desired effect.

Short of actually running the code myself, I'd say this patch looks dandy. [Again,] Nice work!

This revision is now accepted and ready to land.Oct 23 2015, 8:54 PM
mahrens edited edge metadata.Oct 24 2015, 5:09 AM

This is a good start, but you also need to update the manpage and help message (see get_usage()).

Once this is in FreeBSD, would you consider upstreaming it to OpenZFS? (https://github.com/openzfs/openzfs)

allanjude updated this revision to Diff 9687.Oct 24 2015, 7:06 PM
allanjude edited edge metadata.

Updated with feedback from mahrens

add support for -d, to limit depth of search
add support for -p, to print values as literals (raw timestamp instead of date text)
update usage message
update man page

This revision now requires review to proceed.Oct 24 2015, 7:06 PM
bapt accepted this revision.Oct 24 2015, 7:07 PM
bapt edited edge metadata.
This revision is now accepted and ready to land.Oct 24 2015, 7:07 PM

This is a good start, but you also need to update the manpage and help message (see get_usage()).

done (and more)

Once this is in FreeBSD, would you consider upstreaming it to OpenZFS? (https://github.com/openzfs/openzfs)

Of course.

mahrens added inline comments.Oct 25 2015, 4:58 AM
cddl/contrib/opensolaris/cmd/zfs/zfs.8
275–278 ↗(On Diff #9687)

I think you should also add a detailed description of the new -H, -p, and -d flags. (feel free to copy/paste the descriptions from "zfs list").

allanjude updated this revision to Diff 9699.Oct 25 2015, 5:18 PM
allanjude edited edge metadata.

Add missing text to man page

This revision now requires review to proceed.Oct 25 2015, 5:18 PM
mahrens accepted this revision.Oct 25 2015, 7:51 PM
mahrens edited edge metadata.
This revision is now accepted and ready to land.Oct 25 2015, 7:51 PM
bapt accepted this revision.Oct 25 2015, 8:56 PM
bapt edited edge metadata.
smh accepted this revision.Oct 25 2015, 11:25 PM
smh edited edge metadata.
This revision was automatically updated to reflect the committed changes.