Evan Layton wrote:
> Hi Joe,
>
> I've answered a few things below but I'll let Tim and Ethan hit the rest.
>
answering the rest ...
> Thanks again for your comments!
>
> -evan
>
> Joseph J VLcek wrote:
>
> >
> > usr/src/lib/libbe/be_create.c
> >
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
> >
> > 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?
>
No, the check isn't happening, hence the comment. Its a rare case, but it
is possible that a subordinate dataset snapshot has a clone manually created
by the user that the root dataset doesn't have.
The outcome of what the code is doing now isn't crucial and doesn't impact
the result at all; i.e. right now we try to blindly destroy the
snapshot, and
if that fails because a clone exists, we're okay with it and just continue
because that's not a failure case for us anyway.
The comment is noting that we should instead check if the snapshot has
a clone first, and if it does, we shouldn't *try* to destroy it at all
and just
continue.
I'll file a bug for this so that we track it.
> >
> > Issue:
> > ------
> >
> > I think a ZFS_CLOSE(zhp) is needed before the following 2 returns.
> >
> > 1535 return (0);
>
Actually, I'm going to move the entire chunk from 1532-1536 up to
line 1503 where the zfs_handle isn't open yet.
> > 1586 return (ret);
>
I'm going to add a ZFS_CLOSE(zhp) at 1528. Its not used after
that point.
> >
> > 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) {
>
Its needed because it has to "refresh" the zhp handle. Since the call
at 1630
does destructive things to zhp's children (it actually destroy's zhp's
children
datasets), we need to reget the handle to have fresh data.
> >
> > Issue/Question:
> > ---------------
> >
> > 1715 ZFS_CLOSE(zhp);
> > 1731 ZFS_CLOSE(zhp);
> >
> > Does be_destroy_zone_roots_callback() need to issues ZFS_CLOSE(zhp)
> > on error?
>
Yes it does. And I just noticed, the ZFS_CLOSE(zhp) at 1731 needs to
be moved out to 1733 so that the handle gets closed for all cases of
success.
Thanks.
> >
> >
> > 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.
>
Actually, it is possible. The call to z_zones_are_implemented() does not
depend on the z_set_zone_root() having set the zone root.
We'll make this change.
> >
> > 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.
>
>
> >
> > 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.
>
Yeah, it is getting a little inconsistent. I'll add be_ prefixes for
some of the
function to make them consistent. We'll likely not separate out
prefixes for
public, semi-private, and private functions at this point though.
Thanks for the nit :-)
> >
> > 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
>
okay.
> >
> > 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"
>
Its supposed to be a "for"
> >
> > Issue/Suggestion:
> > -----------------
> >
> > Suggestion: use strncmp vs strcmp
> > 497 if (strcmp(active_str, "on") == 0)
>
Is there any real benefit for strncmp in this case?
> > 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?
>
When using typedef to define a new object, the source object definition
doesn't have to be known. The typedef just creates an alias, and for our
case here, it is so that consumers of the api know nothing about the
internal
workings of the real object. Consumers then use accessor functions to
process their new object instance.
Hope that answered your question.
> >
> > 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_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
>
That's correct. Thanks.
thanks,
-ethan
