[caiman-discuss] Code review for 1267

[Ethan Quach]

Jean,


The changes you've made look fine; I've got just one comment
that Tim and Evan have already given you.  However, there's
more to this defect that's not been addressed by your changes.
The intention of this defect was to fix all of the *_callback
functions.  They all need to close the handles passed into
them before returning.  For example, in be_create.c,
be_clone_fs_callback() needs to zfs_close(zhp) before returning.

The caveat is that in a few places we're calling some of these
callbacks directly from our code, and we're expecting the handles
passed in to still be open upon return.  The callbacks that get
called directly are:

be_demote_callback
be_destroy_callback
be_clone_fs_callback
be_send_fs_callback
be_rollback_check_callback
be_rollback_callback


We need to either:

    a) be careful in understanding that the handle is now closed,
and if the code needs to use that handle again after having
called a callback directly, it needs to reopen it. For example,
be_create.c::928 directly calls be_clone_fs_callback().  Upon
return, zhp is closed, and we need to make sure we're reopening
it before its reuse at 966.

    b) reorganize the code such that we're not calling any of the
callbacks directly.



The remaining comments are not in the scope of this defect but
are fairly simple and in the area of your changes, so I figured
I'd nudge them in.. :-)  Its okay if you choose not to do these
though, we'll file separate bugs for them if that's the case.


be_create.c:
-----------
1419,1422,1440,1446,1462,1466 - It doesn't appear that zhp_ss or
ss are used in be_send_fs_callback() for any real reason anymore,
so they should be removed all together from that function.

(I believe they were leftover from a time when
be_send_fs_callback() used to do the actual snapshotting of the
dataset before sending.  That's not the case anymore so they
are just cruft now.)


be_list.c:
---------
244 - Using a failed zfs_open() to determine whether a dataset
exists is wrong.  A !0 return could mean lots of things, one of
which is that the dataset doesn't exist, but we don't know that.
If our code needs to know this logic, we should check that first
by calling zfs_dataset_exists().  So the more proper thing to do
at 244 is:

	if (!zfs_dataset_exists(g_zfs, be_container_ds,
	    ZFS_TYPE_FILESYSTEM)) {
		/*
		 * No BE root ...
		 */
		return (0);
	}

	if ((zhp = zfs_open(g_zfs, be_container_ds,
	    ZFS_TYPE_FILESYSTEM)) == NULL) {
		be_print_err(gettext("be_get_pools: failed to "
                     "open BE container dataset %s: %s\n"),
		    bt.obe_root_ds, libzfs_error_description(g_zfs));
                 return (1);
         }

317 - Same comment.


thanks,
-ethan


Jean McCormack wrote:
> Ethan or Evan,
> 
> Please review the following
> 
> webrev:   http://cr.opensolaris.org/~jeanm/snap_cleanup
> 
> defect: http://defect.opensolaris.org/bz/show_bug.cgi?id=1267
> 
> Thanks,
> Jean
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss