Joe,
Thanks for the review.
Comments interspersed
>
>
>
> 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" ?
>
Yes. changed in the non-pkg code.
> 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.
It only takes 2 positional parameters instead of 3. e.g. in messages.py we have
this message for BE_ERR_MOUNTED which takes 3 parameters:
mBeadmErr[BEADM_ERR_MOUNTED] = "Unable to destroy %(0)s.\nIt is currently
mounted and must be unmounted before it can be destroyed.\nUse 'beadm unmount
%(1)s' to unmount the BE before destroying\nit or 'beadm destroy -fF %(2)s'."
>
> Please confirm this is correct.
>
It is.
>
> 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
>
Yes that is a good suggestion but since the pkgcmds are going away with
Moriah's change I'll avoid making this change.
> 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
>
Ditto comment above.
> 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
>
Ditto comment above.
> 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
>
>
Ditto comment above.
I think Evan addressed the rest of your comments.
Thanks
Tim
> 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
>
>
