Hi Joe,
I've answered a few things below but I'll let Tim and Ethan hit the rest.
Thanks again for your comments!
-evan
Joseph J VLcek wrote:
<...removed stuff already covered...>
>
> usr/src/lib/libbe/be_create.c
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
<...removed stuff already covered...>
>
>
> Issue:
> ------
>
> 1365 /* Get handle to BE's root dataset */
> Please add comment why the 2nd zfs_open is needed.
This is needed because the callback function above the second zfs_open() closes
the zfs handle.
>
> 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.
That's not really possible because of the need to make sure that the dataset is
mounted and that the zone root is set relative to the mount point for the
dataset. In order to do this we have to first do the zfs_open, make sure it's
mounted, get the mount point and then we can call z_set_zone_root() with that
mountpoint so the libinstzones knows where to look for all of it's zones calls.
>
> 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.
Looking at the way it's used, within zfs_iter_filesystems this works because of
the way the zfs_iter functions work. The zfs_iter functions attempt to zfs_open
the datasets as it iterates through the hierarchical children if the dataset
exists we get 1 returned if not then the iter function will return 0 signifying
that there is nothing to process.
>
> 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
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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);
It's expected that the callback functions will close the zfs_handle. This is
because of there use with the zfs_iter functions.
>
> 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
Will add...
>
> 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))) {
strlen(list) better not work since it's a linked list ;-)
>
>
> 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
>
>
