Re: [zones-discuss] Code review for zones support in SNAP (updated)
Joe, Thanks. We'll make that correction. -ethan Joseph J VLcek wrote: > Ethan Quach wrote: >> Joe, Jean, Jerry, >> >> >> Thanks for providing your comments. We've built an incremental webrev >> with just the changes from your comments. This webrev also includes a >> couple of dev bugfixes that went in during the code review process - >> 3776, 3772 >> Any further comments are welcomed. >> >> http://cr.opensolaris.org/~equach/webrev.snap_zones.2 >> >> >> As a reference, we've also refreshed the overall webrev here: >> >> http://cr.opensolaris.org/~equach/webrev.3686 >> >> >> Thanks again for the review, >> >> -ethan > > > This all looks good. I only found one small nit. > > Joe > > >usr/src/lib/libbe/be_create.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+ > > > Issue: > -- > > Typo-s in comment: > > Change from: > 1004 * root dataset of the new BE. The uuid is use to set the parentbe > 1005 * property for the new zones datasets. > To: > 1004 * root dataset of the new BE. The uuid is used to set the parent be > 1005 * property for the new zones datasets. ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Code review for zones support in SNAP (updated)
Ethan Quach wrote: > Joe, Jean, Jerry, > > > Thanks for providing your comments. We've built an incremental webrev > with just the changes from your comments. This webrev also includes a > couple of dev bugfixes that went in during the code review process - > 3776, 3772 > Any further comments are welcomed. > > http://cr.opensolaris.org/~equach/webrev.snap_zones.2 > > > As a reference, we've also refreshed the overall webrev here: > > http://cr.opensolaris.org/~equach/webrev.3686 > > > Thanks again for the review, > > -ethan This all looks good. I only found one small nit. Joe usr/src/lib/libbe/be_create.c +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+ Issue: -- Typo-s in comment: Change from: 1004 * root dataset of the new BE. The uuid is use to set the parentbe 1005 * property for the new zones datasets. To: 1004 * root dataset of the new BE. The uuid is used to set the parent be 1005 * property for the new zones datasets. ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Code review for zones support in SNAP
Ethan Quach wrote: > > Comments, mostly on libbe ... > > > General comment - Since we don't yet support boot environments > within a zone, beadm should fail gracefully when run in a zone, like: > ># beadm >beadm is not supported in a zone. > > rather then trying to do something and failing miserably. Most of > the if(GLOBAL_ZONEID) checks we have in libbe now are really > just primers for when we build in that support. > > > usr/src/man/beadm.1m.txt > -- > 333 - SUNWinstall -> SUNWbeadm > > > messages.py > > 126 - BE_ERR_NO_ACTIVE_ROOT should be in here. > > > be_activate.c > - > 136 - initialize to { 0 } fixed > > 169-199 - This whole chunk needs to be encapsulated inside a >if (GLOBAL) { > >} moved > > 215-221, 241-245 - These two chunks seem like they could be > moved up into the if (GLOBAL) {} from 169. moved > > With some refactoring, the zfs_promote() calls in set_canmount() > could probably be ripped out and replaced by calls to be_promote_zone_ds() > (renamed to something more generic like be_promote_ds() of course). > I never understood why the zfs_promote() calls were hidden in the > set_canmount() function anyway. If we don't get to this per the code > review changes, please file a bug to track this. refactored this so that the promote's are now done with calls to be_promote_ds_callback and are no longer in set_callback (where they never really should have been in the first place...). > > 972-1023 - This whole chunk can be removed if you replace 1025-1026 > with a direct call to the callback giving it the root dataset of the > zone BE. > >i.e. replace 1025-1026 with: > >if (be_promote_sub_ds_callback(z_zhp, NULL) != 0) { done > > 1028 - remove "subordinate" > > Rename the following functions to be more generic: >be_promote_zone_ds() --> be_promote_ds() >be_promote_sub_ds_callback() --> be_promote_ds_callback() I renamed be_promote_sub_ds_callback() to be_promote_ds_callback() However I left be_promote_zone_ds() as is since it's doing a bunch of zones lookup to find the zones and the correct zones datasets to promote before calling be_promote_ds_callback(). > > > > be_create.c: > --- > 1880 - future fixed > > 1878-1883 - this comment needs to be removed. It doesn't apply to > this section of code. fixed > > 1884 - within the context of this function - that is, code running in the > global zone that's processing non-global zones - this if() can be removed, > along with 1895. I removed them. > > 1944 - A comment stating this is getting the uuid of the newly cloned > parent BE would be nice. comment added > > 2772-2774 - Missing function block comment. > > > libbe.h: > - > 117 - "not" -> "no" > 177 - Remove this line; its not used anywhere. > > > > thanks, > -ethan > > > > Evan Layton wrote: >> Hello All, >> >> We're down to the wire on the zone support changes to SNAP upgrade and >> are looking for code review comments. We'll be taking comments up >> until COB Tuesday October 7th. Your comments are as always welcome and >> appreciated. >> >> Defect 3686 is the blocker bug that was submitted to cover this work >> and the webrev is available at: >> >> http://cr.opensolaris.org/~equach/webrev.snap_zones/ >> >> Thank you in advance for your comments and help! >> -evan >> ___ >> zones-discuss mailing list >> zones-discuss@opensolaris.org >> ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Code review for zones support in SNAP
Evan Layton wrote: > Hello All, > > We're down to the wire on the zone support changes to SNAP upgrade and are > looking for code review comments. We'll be taking comments up until COB > Tuesday > October 7th. Your comments are as always welcome and appreciated. > > Defect 3686 is the blocker bug that was submitted to cover this work and the > webrev is available at: > > http://cr.opensolaris.org/~equach/webrev.snap_zones/ > > Thank you in advance for your comments and help! > -evan > ___ > zones-discuss mailing list > zones-discuss@opensolaris.org usr/src/lib/libbe/be_create.c: line 1306 - the goto isn't needed line 1381 - We're not collecting any errors that may have been returned from be_get_snap and just returning a generic BE_ERR_ZFS be_errno. Can we collect any useful information from errors returned by this call? line 1420 - We're not capturing any errors here and just returning whatever "ret" was set to previously, which in this case appears to be 0. line 1519 - We should be collecting the error returned from _be_mount instead of returning the generic mount error. line 1539 - same as 1519 but for unmount. line 1635 - Why not just "goto done" instead of the ZFS_CLOSE and return? unless the callback function has already closed the zfs handle. line 1638 - shouldn't the callback function be_destroy_zone_roots_callback have already closed the zfs handle? line 1705 - shouldn't this callback function always close the zfs handle? line 1719 - if the comparison fails we don't close the zfs handle here. line 2772 - We need a comment header for this function. moving on to be_mount.c -evan ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] Code review for zones support in SNAP
Comments, mostly on libbe ... General comment - Since we don't yet support boot environments within a zone, beadm should fail gracefully when run in a zone, like: # beadm beadm is not supported in a zone. rather then trying to do something and failing miserably. Most of the if(GLOBAL_ZONEID) checks we have in libbe now are really just primers for when we build in that support. usr/src/man/beadm.1m.txt -- 333 - SUNWinstall -> SUNWbeadm messages.py 126 - BE_ERR_NO_ACTIVE_ROOT should be in here. be_activate.c - 136 - initialize to { 0 } 169-199 - This whole chunk needs to be encapsulated inside a if (GLOBAL) { } 215-221, 241-245 - These two chunks seem like they could be moved up into the if (GLOBAL) {} from 169. With some refactoring, the zfs_promote() calls in set_canmount() could probably be ripped out and replaced by calls to be_promote_zone_ds() (renamed to something more generic like be_promote_ds() of course). I never understood why the zfs_promote() calls were hidden in the set_canmount() function anyway. If we don't get to this per the code review changes, please file a bug to track this. 972-1023 - This whole chunk can be removed if you replace 1025-1026 with a direct call to the callback giving it the root dataset of the zone BE. i.e. replace 1025-1026 with: if (be_promote_sub_ds_callback(z_zhp, NULL) != 0) { 1028 - remove "subordinate" Rename the following functions to be more generic: be_promote_zone_ds() --> be_promote_ds() be_promote_sub_ds_callback() --> be_promote_ds_callback() be_create.c: --- 1880 - future 1878-1883 - this comment needs to be removed. It doesn't apply to this section of code. 1884 - within the context of this function - that is, code running in the global zone that's processing non-global zones - this if() can be removed, along with 1895. 1944 - A comment stating this is getting the uuid of the newly cloned parent BE would be nice. 2772-2774 - Missing function block comment. libbe.h: - 117 - "not" -> "no" 177 - Remove this line; its not used anywhere. thanks, -ethan Evan Layton wrote: > Hello All, > > We're down to the wire on the zone support changes to SNAP upgrade and are > looking for code review comments. We'll be taking comments up until COB > Tuesday > October 7th. Your comments are as always welcome and appreciated. > > Defect 3686 is the blocker bug that was submitted to cover this work and the > webrev is available at: > > http://cr.opensolaris.org/~equach/webrev.snap_zones/ > > Thank you in advance for your comments and help! > -evan > ___ > zones-discuss mailing list > zones-discuss@opensolaris.org > ___ zones-discuss mailing list zones-discuss@opensolaris.org