[caiman-discuss] Code review for 1267

[Jean McCormack]

Ethan Quach wrote:
> 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.
Oh. I see. I'll make the necessary modifications. Sorry, I just didn't
get that out of the bug report.
>
>
>
> 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.)
Easy to nudge in. I'll do these.
>
>
> 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.
>
>
these seem easy too. I'll probably modify the synopsis of the bug report
to be a bit more generic so I can justify putting these fixes in too.

Thanks for the review,

Jean
> 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