Joseph J VLcek wrote:
> 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
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
> This exchange had accidentally happened off caiman-discuss...
>
> replying to original code review request...
>
> Joe
>
> Evan Layton wrote:
> > Hi Folks,
> >
> > We are in desperate need of help in getting our code review done so
> we're begging for help. Would any of you have some time to help out with
> this code review. I know it's rather large but we'd split things up so
> no one has to do too much of it. Please let us know if you can help!
> >
> > Joe, could you let us know how much of this you've already looked at
> so we add that in to how we split this up?
> >
> > Thanks,
> > -evanT
>
<...>
>
>
> My comments so far are:
> -----------------------
>
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
> General Issues/Questions:
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> Genral Issue 1:
> ---------------
>
> Regarding #include "instzones_api.h"
>
> Why in some places:
> #include "instzones_api.h"
> And in others:
> #include <instzones_api.h>
>
>
> Shouldn't they all be #include "instzones_api.h" ?
>
> Both will work but...
>
> General Issue 2:
> ----------------
>
> An idea to consider
>
> Instead of relying on the caller to check if getzoneid() == GLOBAL_ZONEID
> before invoking the global zone specific be_ functions perhaps it would
> be safer to have those be_ functions which are global zone specific
> make the check. ???
>
> This way if one is accidently invoked from a non-global zone they won't
> attempt to do something they shouldn't be.
>
> e.g. have be_append_grub() check if (getzoneid() == GLOBAL_ZONEID)
> instead of doing it before calling be_append_grub()
>
>
> usr/src/cmd/beadm/beadm.py
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> A question:
> -----------
> 287 be.msgBuf["1"] = be.trgtBeNameOrSnapshot[0]
>
> When handling error BE_ERR_MOUNTED both be.msgBuf["1"] and be.msgBuf["2"]
> are set. Why is only be.msgBuf["1"] set for error BE_ERR_ZONES_UNMOUNT.
>
> Please confirm this is correct.
>
>
> usr/src/cmd/beadm/messages.py
>
<...snip...>
>
> Suggestion:
> ----------
>
> In function:
> 130 _be_activate(char *be_name)
>
> perhaps calling getzoneid() once and saving the return would be better
> than invoking it multiple times.
I've moved all of these into one if clause so it's only called once for most of
the function. I did leave the call to this before the zone dataset promotions
because I wanted that after the dataset promotions for the global BE.
>
>
> Question/please confirm:
> ------------------------
> In usr/src/lib/libbe/be_activate.c is it OK to invoke be_do_installgrub()
> from a non-global zone?
Definitely not! Nice catch. I've moved this up with the rest of the code after
the first chack for getzoneid == GLOBAL_ZONEID.
>
> e.g.:
> 181 err = be_do_installgrub(&cb);
>
> Issue:
> ------
> Parameter comment does no match parameters
>
> 872 * Parameters:
> 873 * zhp - the zfs handle for global zone BE being
> activated.
oops! fixed.
>
>
> Issue:
> ------
> temp_mntpt and brands list need to be cleaned up prior to return on line:
>
> 943 return (0);
brands is getting cleaned up on line 942 but I did miss freeing temp_mntpt.
Fixed.
>
> Issue:
> ------
> Suggestion: use strncmp vs strcmp
> 994 while (origin[0] != '\0' && strcmp(origin, "-") != 0) {
> 1096 while (origin[0] != '\0' && strcmp(origin, "-") != 0) {
994 is gone now but 1096 is still there in be_promote_zone_ds_callback
leaving it for now...
>
> However since the string being compared is not a variable but instead
> hardcoded it is probably OK to leave it.
>
> usr/src/lib/libbe/be_create.c
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> General question:
> -----------------
>
> no change needed unless you want to
>
> Why do you use the be_ prefix for some static functions and then not
> for get_zone_be_name() ?
>
> I thought the library prefix wasn't to be used on static stuff.
>
> Issue:
> ------
> Please confirm this is correct.
>
> After at line 475:
>
> 475 if ((ret = be_get_uuid(zfs_get_name(zhp), &dd.gz_be_uuid)) != 0) {
>
> if be_get_uuid fails an error is printed then the code continues.
>
> Should it clean up and return or goto done between lines 477 and 478?
older BE's (pre build 96 I think) didn't have uuid's accociated with them but
we
still need to handle those BE's.
>
>
> Issue:
> ------
> Should ZFS_CLOSE(zhp); be invoked after line the done lable at line 539?
>
>
> 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.
The property name is parentbe, well actually org.opensolaris.libbe:parentbe but
I didn't want to write out the whole thing...
>
>
> I still have to review from line 1336 down.
> I still have to review from line 1336 down.
> I still have to review from line 1336 down.
>
