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.
Tags
None
Referenced Files
Unknown Object (File)
Oct 23 2024, 1:11 AM
Unknown Object (File)
Oct 9 2024, 9:19 AM
Unknown Object (File)
Oct 5 2024, 12:19 AM
Unknown Object (File)
Oct 1 2024, 7:53 PM
Unknown Object (File)
Sep 29 2024, 12:50 PM
Unknown Object (File)
Sep 28 2024, 5:31 PM
Unknown Object (File)
Sep 27 2024, 5:35 AM
Unknown Object (File)
Sep 24 2024, 5:45 AM
Subscribers

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 - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 16012
Build 15991: arc lint + arc unit

Event Timeline

cddl/contrib/opensolaris/cmd/zpool/zpool.8
1396

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

Missing -t addition

236

Missing -t addition

1024

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

1029

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.

2196

Missing -t additions

2482–2483

Should we mention -t here also?

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c
672

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

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c
4669

Consistency poolname here vs pool_name everywhere else?

4672

zpool_prop_to_name?

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
253

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

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

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
1024

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

2482–2483

seems like a good idea

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c
672

good suggestion

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c
4669

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

Good suggestion!

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_config.c
253

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

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