Allow for '-o feature@<feature>=disabled' on the command line. #5324

loli10K · 2016-10-22T18:01:03Z

Based on @FransUrbo original work from previous PR #3465.

Enables new test script zpool_create_features_005_pos in Linux runfile and updates existing zpool_create_features_004_neg test case.

In case of invalid property value libzfs still emits a misleading error "cannot create 'tank': property 'feature@async_destroy' can only be set to 'enabled'" even if now 'disabled' is also accepted ...

Should fix #3460.

mention-bot · 2016-10-22T18:01:06Z

@loli10K, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ahrens, @behlendorf and @ryao to be potential reviewers.

behlendorf · 2016-10-24T22:22:06Z

tests/zfs-tests/tests/functional/cli_root/zpool_create/zpool_create_features_005_pos.ksh

+
+for feature in async_destroy bookmarks embedded_data empty_bpobj enabled_txg \
+               extensible_dataset filesystem_limits hole_birth large_blocks  \
+               lz4_compress spacemap_histogram


We're the large_dnode and userobj_accounting intentionally skipped?

behlendorf · 2016-10-24T22:25:11Z

man/man8/zpool.8

-     ... [\fB-m\fR \fImountpoint\fR] [\fB-R\fR \fIroot\fR] [\fB-t\fR \fItname\fR] \fIpool\fR \fIvdev\fR ...
+\fBzpool create\fR [\fB-fnd\fR] [\fB-o\fR \fIproperty=value\fR] ... [\fB-o\fR feature@\fIfeature=value\fR]
+     ... [\fB-O\fR \fIfile-system-property=value\fR] ... [\fB-m\fR \fImountpoint\fR] [\fB-R\fR \fIroot\fR] [\fB-t\fR \fItname\fR]
+     ... \fIpool\fR \fIvdev\fR ...


Can you fix the line wrapping here. [-t name] needs to be moved to the last line with ... pool vdev ...

zpool create [-fnd] [-o property=value] ... [-o feature@feature=value] ... [-O file-system-property=value] ... [-m mountpoint] [-R root] [-t tname] ... pool vdev ...

behlendorf

Nice job. This is a nice clean way to handle this, I didn't run in to any issues testing it locally aside from the error message. Is there some reason you didn't take this in the original patch? Aside from the two minor inline comments that's the only remaining issue I see holding up merging this.

behlendorf · 2016-10-24T22:31:46Z

Superceeds #3465. @FransUrbo, @gordan-bobic, @DeHackEd if you get a chance can you please review and test this.

loli10K · 2016-10-24T22:37:27Z

Is there some reason you didn't take this in the original patch?

I just wanted to stay as close as possible to the original PR, i will update that error message in libzfs along with the other stuff.

EDIT: @behlendorf should i also add feature@sha512 feature@skein and feature@edonr?

behlendorf · 2016-10-24T23:27:29Z

@loli10K yes, we might as well add the full set of supported features.

loli10K · 2016-10-24T23:34:32Z

Changes up. Also i just realized this should probably fix #5142.

Sometimes it is desired to specifically disable a feature directly on the 'zpool create' command line. Signed-off-by: Turbo Fredriksson <turbo@bayour.com> Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

Updated the test case so it behaves in the manor originally indended. * Consistent sytle with other test cases * log_pass causes immediate exit, moved outside loop * Simplified check_features function * log_must added to check_features call * Several features removed to keep test time short Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

behlendorf · 2016-10-25T20:01:07Z

@loli10K the patch itself looks good but the test case wasn't working as you originally intended. I've added commit b06b75e to this branch which addresses the issues with the test case. Please review those changes.

loli10K · 2016-10-25T20:21:19Z

LGTM, but now i wonder if we should also test multiple -o feature@<feature>=disabled|enabled with various permutations to exercise the cli parsing code of this patch?

EDIT: i can't review the code, Github says i can't approve my own PR.

behlendorf · 2016-10-25T20:26:50Z

@loli10K that would be a nice addition but I don't think it's critical. The parsing logic is pretty straight forward and I did manually run some of those kind of tests and it did work as expected. Thanks for the quick review.

behlendorf · 2016-10-25T23:20:30Z

@loli10K thanks for tackling this, I know this is something quite a few people were requesting.

loli10K · 2016-10-26T07:33:14Z

@behlendorf that's the reason i did it, i saw different issues being open for this and i thought someone had to implement it.

I know i'm being offtopic here, but do you happen to know if some other developer is working on recv property overrides (-x|-o)?

behlendorf · 2016-10-26T16:19:53Z

@loli10K I don't know of anyone working on the recv property overrides. There was a WIP patch in 1867 along with a first round of review comments.

ahrens · 2016-10-26T16:41:31Z

On the recv property overrides, IIRC my comments were essentially, "why are the semantics subtly different from those of Oracle Solaris"?

loli10K · 2016-10-26T16:46:50Z

@ahrens yes, that's exactly as i read them. Is anyone in the Illumos community working on this? The open issue here https://www.illumos.org/issues/2745 doesn't show much activity either ...

ryao · 2016-10-27T03:18:10Z

@loli10K I can confirm @behlendorf's remark about not knowing anyone working on recv property overrides in the Linux community. I don't know of anyone either.

behlendorf requested changes Oct 24, 2016

View reviewed changes

behlendorf mentioned this pull request Oct 24, 2016

Allow for '-o feature@<feature>=disable' on the command line. #3465

Closed

loli10K force-pushed the issue-3460 branch from ca3fdfb to b994fcf Compare October 24, 2016 23:32

Allow for '-o feature@<feature>=disabled' on the command line.

a56e5d6

Sometimes it is desired to specifically disable a feature directly on the 'zpool create' command line. Signed-off-by: Turbo Fredriksson <turbo@bayour.com> Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

loli10K force-pushed the issue-3460 branch from b994fcf to a56e5d6 Compare October 24, 2016 23:36

behlendorf approved these changes Oct 25, 2016

View reviewed changes

behlendorf merged commit e4010f2 into openzfs:master Oct 25, 2016

loli10K deleted the issue-3460 branch October 26, 2016 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for '-o feature@<feature>=disabled' on the command line. #5324

Allow for '-o feature@<feature>=disabled' on the command line. #5324

loli10K commented Oct 22, 2016

mention-bot commented Oct 22, 2016

behlendorf Oct 24, 2016

behlendorf Oct 24, 2016

behlendorf left a comment

behlendorf commented Oct 24, 2016 •

edited

loli10K commented Oct 24, 2016 •

edited

behlendorf commented Oct 24, 2016

loli10K commented Oct 24, 2016

behlendorf commented Oct 25, 2016

loli10K commented Oct 25, 2016 •

edited

behlendorf commented Oct 25, 2016

behlendorf commented Oct 25, 2016

loli10K commented Oct 26, 2016

behlendorf commented Oct 26, 2016

ahrens commented Oct 26, 2016

loli10K commented Oct 26, 2016

ryao commented Oct 27, 2016

Allow for '-o feature@<feature>=disabled' on the command line. #5324

Allow for '-o feature@<feature>=disabled' on the command line. #5324

Conversation

loli10K commented Oct 22, 2016

mention-bot commented Oct 22, 2016

behlendorf Oct 24, 2016

Choose a reason for hiding this comment

behlendorf Oct 24, 2016

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Oct 24, 2016 • edited

loli10K commented Oct 24, 2016 • edited

behlendorf commented Oct 24, 2016

loli10K commented Oct 24, 2016

behlendorf commented Oct 25, 2016

loli10K commented Oct 25, 2016 • edited

behlendorf commented Oct 25, 2016

behlendorf commented Oct 25, 2016

loli10K commented Oct 26, 2016

behlendorf commented Oct 26, 2016

ahrens commented Oct 26, 2016

loli10K commented Oct 26, 2016

ryao commented Oct 27, 2016

behlendorf commented Oct 24, 2016 •

edited

loli10K commented Oct 24, 2016 •

edited

loli10K commented Oct 25, 2016 •

edited