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 [EMAIL PROTECTED] http://mail.opensolaris.org/mailman/listinfo/caiman-discussThis 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 goodusr/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
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
General 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 OK
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 OK
usr/src/cmd/pkgcmds/pkgadd/check.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgadd/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment is wrong.
63 * libspmi includes
usr/src/cmd/pkgcmds/pkgadd/quit.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgcond/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkginfo/pkginfo.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkginstall/instvol.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment is wrong.
53 * libspmi includes
usr/src/cmd/pkgcmds/pkginstall/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgmk/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgremove/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgrm/check.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/pkgcmds/pkgrm/main.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment is wrong.
51 * libspmi includes
usr/src/cmd/pkgcmds/pkgrm/quit.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libbe/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
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.
Issue:
------
1365 /* Get handle to BE's root dataset */
Please add comment why the 2nd zfs_open is needed.
Issue/Please confirm:
---------------------
1446 * has a clone. We really need to check for that
The comment indicates a check if a subordinate dataset has a clone
is needed but it's not clear to me if that check is happening?
Issue:
------
I think a ZFS_CLOSE(zhp) is needed before the following 2 returns.
1535 return (0);
1586 return (ret);
Issue/Question:
---------------
Is the zfs_open at line 1641 necessary?
Or could the close at line 1638 be left out?
1638 ZFS_CLOSE(zhp);
1639
1640 /* Get handle to this zone's root container dataset. */
1641 if ((zhp = zfs_open(g_zfs, zone_container_ds, ZFS_TYPE_FILESYSTEM))
1642 == NULL) {
Issue/Question:
---------------
1715 ZFS_CLOSE(zhp);
1731 ZFS_CLOSE(zhp);
Does be_destroy_zone_roots_callback() need to issues ZFS_CLOSE(zhp)
on error?
Issue/Suggestion:
-----------------
Suggestion: Move linse 1809 -> 1813 before line 1778
I suggest moving the z_zones_are_implemented check to the
beginning of be_copy_zones. Might as well do that first.
Issue/Please confirm:
---------------------
2798 be_zone_root_exists_callback(zfs_handle_t *zhp, void *data)
How is this doing anything?
Please check that this is doing the correct thing and add to the
comment how closing zfs will confirm a hierarchical child of the
zone root container dataset has been traversed and therefore it
has children.
usr/src/lib/libbe/be_mount.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
I did not review this. It was reviewed by Jean and Tim.
usr/src/lib/libbe/be_rename.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
I did not review this. It was reviewed by Jean.
usr/src/lib/libbe/be_utils.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
I did not review this. It was reviewed by Jean.
usr/src/lib/libbe/libbe.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libbe/libbe_priv.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libbe/tbeadm/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinst/pkgdbmerg.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinst/pkgops.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinst/putparam.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/liborchestrator/test_kbd.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmiapp/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmiapp/app_upgrade.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libspmiapp/app_utils.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libspmisoft/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisoft/soft_install.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisoft/soft_sp_load.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisoft/soft_sp_space.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisoft/soft_update_actions.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisoft/spmisoft_lib.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmistore/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/spmisvc_lib.h
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/svc_mountall.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/svc_services.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/svc_updatesys.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/svc_upgradeable.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libspmisvc/svc_write_script.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libtd/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libtd/td_mg.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libtd/td_mountall.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWinstall-libs/prototype_com
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/pkgdefs/SUNWinstall/prototype_com
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
JJV ?
New usr/src/lib/libbe/be_zones.c
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit:
----
Throughout the code the be_ prefix is sometimes and not other.
consistency might help with maintenance .
Perhaps the semi-private Libarary use only could use bel_ and the
global functions could use be_ and static functions use no prefix.
Issue:
------
already already repeated repeated in comment comment ;)
409 * Found active zone root dataset, if its already
410 * already set in the callback data, that means this
Issue:
------
Is the ZFS_CLOSE() at line 423 needed? I thought the caller
was to close it.
423 ZFS_CLOSE(zhp);
Issue:
------
481 /* Get user properties fo the zone root dataset */
Change "fo" to "of"
Issue/Suggestion:
-----------------
Suggestion: use strncmp vs strcmp
497 if (strcmp(active_str, "on") == 0)
usr/src/lib/libinstzones/Makefile
(renamed usr/src/lib/libspmizones/Makefile)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/instzones_api.h
(renamed usr/src/lib/libspmizones/spmizones_api.h)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
83 typedef struct _zoneBrandList zoneBrandList_t;
struct _zoneBrandList is defined in spmizones_lib.h and referenced here
on line 83 but instzones_api.h does not include spmizones_lib.h
This does not seem correct to me. Am I missing something?
usr/src/lib/libinstzones/instzones_lib.h
(renamed usr/src/lib/libspmizones/spmizones_lib.h)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
See above:
usr/src/lib/libinstzones/zones.c
(renamed usr/src/lib/libspmizones/zones.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
1706 * Arguments: zoneName - name of the zone to check for branding
Missing description of argument: list
Issue:
------
1718 if (zoneName == NULL || list == NULL)
What if zoneName or list are not null but empty, e.g. ""?
Suggestion change to:
if (((zoneName == NULL) || (strlen(zoneName) == 0)) ||
((list == NULL) || (strlen(list) == 0))) {
usr/src/lib/libinstzones/zones_args.c
(renamed usr/src/lib/libspmizones/zones_args.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_exec.c
(renamed usr/src/lib/libspmizones/zones_exec.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_locks.c
(renamed usr/src/lib/libspmizones/zones_locks.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_lofs.c
(renamed usr/src/lib/libspmizones/zones_lofs.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
41 #include "instzones_api.h"
I don't think the include of instzones_api.h is necessary because
instzones_lib.h includes instzones_api.h
usr/src/lib/libinstzones/zones_paths.c
(renamed usr/src/lib/libspmizones/zones_paths.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_states.c
(renamed usr/src/lib/libspmizones/zones_states.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_str.c
(renamed usr/src/lib/libspmizones/zones_str.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_strings.h
(renamed usr/src/lib/libspmizones/zones_strings.h)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/lib/libinstzones/zones_utils.c
(renamed usr/src/lib/libspmizones/zones_utils.c)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/Makefile
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/depend
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/pkginfo.tmpl
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/prototype_com
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/prototype_i386
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
New usr/src/pkgdefs/SUNWbeadm/prototype_sparc
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
_______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
