Great Tim!
Sounds good.
Joe
Tim Knitter wrote:
>
>
> 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
>>
>>
