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:
