Evan Layton wrote: > Hi Ethan, > > Overall this looks pretty good. One thing I did wonder about is the use of > the > number 12 for checking the supported ZFS version. I was wondering if it might > be > better to define that in libbe_priv.h so that if in the future we end up with > the need to limit things to another ZFS version we can just change it there. >
Different features will depend on different ZFS versions though, so its not like we'd reuse that define in another area of libbe code that depended on some new ZFS feature introduced in, say version 15 for example. Your comment triggered me to look though and I found version macros in zfs.h, so I'll use SPA_VERSION_SNAP_PROPS instead of 12. thanks, -ethan > Thanks! > > -evan > > > Ethan Quach wrote: > >> Defect: >> --------- >> http://defect.opensolaris.org/bz/show_bug.cgi?id=1197 >> >> Webrev: >> ---------- >> http://cr.opensolaris.org/~equach/webrev.1197 >> >> >> This fix changes the auto snapshot name such that the cleanup policy >> type is no longer stored in the snapshot name. The cleanup policy is >> now stored as a user property in the snapshot dataset itself. >> >> I've successfully tested the cases for providing a non-default cleanup >> policy names, and not providing one at all (which causes fallback to setting >> the default cleanup policy.) >> >> A more interesting scenario is backwards compatibility with existing >> boot environment snapshots that don't have the cleanup policy set in >> a user property. For this case, the code no longer parses the cleanup >> policy >> type out of the snapshot name, and will inherit it from its parent dataset. >> Since auto-cleanup is not implemented yet, the only policy ever set was the >> default, so this shouldn't result in any problems. >> >> An even more interesting scenario is when the pool has not been upgraded >> to a zfs version that supports snapshot user properties. To handle >> this, the >> pool version is always checked to determine if it supports snapshot user >> properties. If it does not, then attempting to set a non-default cleanup >> policy will result in a failure - the user must upgrade their zfs version to >> to be able to do this. Currently, there is no way to set a non-default user >> property (auto-cleanup has not been implemented yet), so this does not >> result in any issues. When auto-cleanup is implemented, users will be >> required to upgrade their pools to a version that supports snapshot user >> properties to use the new feature. >> >> >> thanks, >> -ethan >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
