My completed full list of comments are attached.
I realize some of these have already been address. Hope this helps! Joe 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 > > > > I will plan to look at all the bigger files. > > The bigger files are: > --------------------- > > usr/src/lib/libbe/be_activate.c 291 lines changed: DONE JJV > usr/src/lib/libbe/be_create.c 1012 lines changed: DOING JJV > > usr/src/lib/libbe/be_mount.c 788 lines changed: > usr/src/lib/libbe/be_utils.c 577 lines changed: > usr/src/lib/libbe/libbe_priv.h 28 lines changed: > New usr/src/lib/libbe/be_zones.c 501 lines changed: > usr/src/lib/libinstzones/zones.c (renamed, was > usr/src/lib/libspmizones/zones.c) 216 lines changed: > New usr/src/pkgdefs/SUNWbeadm/Makefile 37 lines changed: > New usr/src/pkgdefs/SUNWbeadm/depend 45 lines changed: > New usr/src/pkgdefs/SUNWbeadm/pkginfo.tmpl 49 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_com 68 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_i386 50 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_sparc 50 lines changed: > > > The list of files I've already reviewed are: > -------------------------------------------- > > usr/src/cmd/beadm/beadm.py > usr/src/cmd/beadm/messages.py > usr/src/cmd/pkgcmds/installf/Makefile > usr/src/cmd/pkgcmds/pkgadd/Makefile > usr/src/cmd/pkgcmds/pkgchk/Makefile > usr/src/cmd/pkgcmds/pkgcond/Makefile > usr/src/cmd/pkgcmds/pkginfo/Makefile > usr/src/cmd/pkgcmds/pkginstall/Makefile > usr/src/cmd/pkgcmds/pkgmk/Makefile > usr/src/cmd/pkgcmds/pkgparam/Makefile > usr/src/cmd/pkgcmds/pkgremove/Makefile > usr/src/cmd/pkgcmds/pkgrm/Makefile > usr/src/cmd/pkgcmds/installf/main.c > usr/src/cmd/pkgcmds/pkgadd/check.c > usr/src/cmd/pkgcmds/pkgadd/main.c > usr/src/cmd/pkgcmds/pkgadd/quit.h > usr/src/cmd/pkgcmds/pkgcond/main.c > usr/src/cmd/pkgcmds/pkginfo/pkginfo.c > usr/src/cmd/pkgcmds/pkginstall/instvol.c > usr/src/cmd/pkgcmds/pkginstall/main.c > usr/src/cmd/pkgcmds/pkgmk/main.c > usr/src/cmd/pkgcmds/pkgremove/main.c > usr/src/cmd/pkgcmds/pkgrm/check.c > usr/src/cmd/pkgcmds/pkgrm/main.c > usr/src/cmd/pkgcmds/pkgrm/quit.c > usr/src/lib/Makefile > usr/src/lib/libbe/Makefile > usr/src/lib/libbe/be_activate.c > usr/src/lib/libbe/be_create.c > usr/src/lib/libbe/tbeadm/Makefile > usr/src/lib/libspmiapp/Makefile > usr/src/lib/libspmisoft/Makefile > usr/src/lib/libspmistore/Makefile > usr/src/lib/libspmisvc/Makefile > usr/src/lib/libtd/Makefile > usr/src/pkgdefs/Makefile > usr/src/lib/libinstzones/Makefile > (renamed usr/src/lib/libspmizones/Makefile) > > > 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 > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/installf/Makefile > usr/src/cmd/pkgcmds/pkgadd/Makefile > usr/src/cmd/pkgcmds/pkgchk/Makefile > usr/src/cmd/pkgcmds/pkgcond/Makefile > usr/src/cmd/pkgcmds/pkginfo/Makefile > usr/src/cmd/pkgcmds/pkginstall/Makefile > usr/src/cmd/pkgcmds/pkgmk/Makefile > usr/src/cmd/pkgcmds/pkgparam/Makefile > usr/src/cmd/pkgcmds/pkgremove/Makefile > usr/src/cmd/pkgcmds/pkgrm/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Where LIBSPMI macro has been changed perhaps the macro name > and all references to it should also change. > > Suggestion from: > LIBSPMI= -linstzones > Suggestion to: > LIBINSTZONE= -linstzones > > usr/src/cmd/pkgcmds/installf/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgadd/check.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgadd/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Comment is wrong. > > 63 * libspmi includes > > usr/src/cmd/pkgcmds/pkgadd/quit.h > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgcond/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkginfo/pkginfo.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkginstall/instvol.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Comment is wrong. > > 53 * libspmi includes > > usr/src/cmd/pkgcmds/pkginstall/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgmk/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgremove/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgrm/check.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/cmd/pkgcmds/pkgrm/main.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Comment is wrong. > 51 * libspmi includes > > > usr/src/cmd/pkgcmds/pkgrm/quit.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libbe/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libbe/be_activate.c > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > 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. > > > Question/please confirm: > ------------------------ > In usr/src/lib/libbe/be_activate.c is it OK to invoke be_do_installgrub() > from a non-global zone? > > 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. > > > Issue: > ------ > temp_mntpt and brands list need to be cleaned up prior to return on line: > > 943 return (0); > > Issue: > ------ > Suggestion: use strncmp vs strcmp > 994 while (origin[0] != '\0' && strcmp(origin, "-") != 0) { > 1096 while (origin[0] != '\0' && strcmp(origin, "-") != 0) { > > 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? > > > 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. > > > 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. > > usr/src/lib/libbe/tbeadm/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libspmiapp/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libspmisoft/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libspmistore/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libspmisvc/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libtd/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/pkgdefs/Makefile > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks good > > usr/src/lib/libinstzones/Makefile (renamed > usr/src/lib/libspmizones/Makefile) > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > Looks Good > > EOF > > > The big files are: > > usr/src/lib/libbe/be_activate.c 291 lines changed: > usr/src/lib/libbe/be_create.c 1012 lines changed: > > usr/src/lib/libbe/be_mount.c 788 lines changed: > usr/src/lib/libbe/be_utils.c 577 lines changed: > usr/src/lib/libbe/libbe_priv.h 28 lines changed: > New usr/src/lib/libbe/be_zones.c 501 lines changed: > usr/src/lib/libinstzones/zones.c (renamed, was > usr/src/lib/libspmizones/zones.c) 216 lines changed: > New usr/src/pkgdefs/SUNWbeadm/Makefile 37 lines changed: > New usr/src/pkgdefs/SUNWbeadm/depend 45 lines changed: > New usr/src/pkgdefs/SUNWbeadm/pkginfo.tmpl 49 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_com 68 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_i386 50 lines changed: > New usr/src/pkgdefs/SUNWbeadm/prototype_sparc 50 lines changed: > > > > My -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: snap_112008.txt URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20081008/9f1ae2c1/attachment.txt>
