Page MenuHomeFreeBSD

allow ZFS pool to have temporary name for duration of current import
ClosedPublic

Authored by avg on Apr 5 2018, 2:05 PM.

Details

Summary

This is a parallel track to https://github.com/openzfs/openzfs/pull/600 to get
more reviews and, possibly, speedier adoption in FreeBSD. Specifically, I
would appreciate any help on improving the manual page changes.

The changes come from ZoL.
The porting has been done by @julian.

The change adds -t <name> option zpool create and -t option to zpool import in
its form with an old name and a new name. This allows to import (or create) a
pool under a name that's different from its real, permanent name without
affecting that name. This is useful when working with VM images or images of
other physical systems if they happen to have a ZFS pool with the same name as
the host system.

Test Plan

Tested various scenarios of zpool create, export, import, export while using
temporary names.

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

avg created this revision.Apr 5 2018, 2:05 PM
smh added inline comments.Apr 5 2018, 9:00 PM
cddl/contrib/opensolaris/cmd/zpool/zpool.8
1396 ↗(On Diff #41129)

This reads odd to me:
Used with newpool. Specifies that newpool is temporary.
I would have expected it to be:
Used with newpool specifies that newpool is temporary.

cddl/contrib/opensolaris/cmd/zpool/zpool_main.c
220 ↗(On Diff #41129)

Missing -t addition

235 ↗(On Diff #41129)

Missing -t addition

1022 ↗(On Diff #41129)

I don't think this bit of the validation from the main pool name doesn't make much sense in this new context

1027 ↗(On Diff #41129)

Personally I don't like using int returns as boolean but seems like all uses as the same in this file so won't object.

2194 ↗(On Diff #41129)

Missing -t additions

2477 ↗(On Diff #41129)

Should we mention -t here also?

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c
661 ↗(On Diff #41129)

case below default is odd, I would move it up.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c
4669 ↗(On Diff #41129)

Consistency poolname here vs pool_name everywhere else?

4672 ↗(On Diff #41129)

zpool_prop_to_name?

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
253 ↗(On Diff #41129)

Declare pool_name here?

smh requested changes to this revision.Apr 5 2018, 9:00 PM
This revision now requires changes to proceed.Apr 5 2018, 9:00 PM
avg updated this revision to Diff 41166.Apr 6 2018, 9:26 AM

Fix add_prop_list_default to not ignore poolprop.
No functional change as all current callers passed B_TRUE in poolprop.
Found by illumos lint.

avg marked 10 inline comments as done.Apr 6 2018, 10:29 AM
avg added inline comments.
cddl/contrib/opensolaris/cmd/zpool/zpool.8
1396 ↗(On Diff #41129)

The wording may be not quite clear, but the suggested alternative is wrong.
The intended meaning is that -t should only be used when newpool is also specified.

cddl/contrib/opensolaris/cmd/zpool/zpool_main.c
1022 ↗(On Diff #41129)

You don't explain why and I do not see why.

2477 ↗(On Diff #41129)

seems like a good idea

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c
661 ↗(On Diff #41129)

good suggestion

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c
4669 ↗(On Diff #41129)

Given that we are taking the already written code from ZoL (as opposed to submitting completely new code) I am a bit (but not strongly) opposed to gratuitous naming changes.

4672 ↗(On Diff #41129)

Good suggestion!

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
253 ↗(On Diff #41129)

No strong preference either way. The existing code seems to use a mix of styles, e.g., see nvl vs spa.

avg updated this revision to Diff 41169.Apr 6 2018, 10:31 AM
avg marked 6 inline comments as done.

address feedback from @smh

  • add -t to comments describing zpool_do_create and zpool_do_import
  • add -t to zpool create and zpool import usage messages
  • add -t as an alternative to permanently renaming a pool in a message that's produced after an attempt to import a pool with the same name as an already active pool
  • re-order a case block to be before a default block
  • use zpool_prop_to_name(ZPOOL_PROP_TNAME) instead of hardcoded "tname"
smh accepted this revision.Apr 6 2018, 4:14 PM
This revision is now accepted and ready to land.Apr 6 2018, 4:14 PM
This revision was automatically updated to reflect the committed changes.