Keith Mitchell wrote:
>
>>
>> line 591 zret should be initialized to 0 (or BE_SUCESS since it's
>> used for both zfs_iter function return codes and be_clone_fs_callback).
> Done.
>> It might be best as part of this to clean up these return variables so
>> we make the use of things like err, zret and ret consistent. This
>> needs to be looked at throughout...
>> Maybe something like:
>> err for capturing errno's
>> zret for capturing zfs related errors
>> ret for returning BE errors and errors that have been translated into
>> BE errors.
> Ok. I'll go through again and clean up these instances.
It's probably not worth having you look through all of these right now but
would
be helpful to have you look at the areas that you've touched only. As for the
rest of these could you file a bug to state that we should be more consistent
with these error variables and attach that to 9941 as well?
>>
>> 1796 ret is defined here and again at line 1886. The second one isn't
>> needed and should be removed.
> Fixed.
>>
>> 2148 - 2150 returns err but doesn't set it so we could be sending an
>> error from somewhere else or just what was initialized. This needs an
>> actual error message.
> err is set at line 2139. However, The inner 'if' statement is kind of
> backwards. I'll change it to:
>
> 2139 if ((err = zfs_iter_filesystems(zhp, be_clone_fs_callback,
> bt)) != 0) {
> 2140 /*
> 2141 * Error occurred while processing a child dataset.
> 2142 * Destroy this dataset and return error.
> 2143 */
> 2144 zfs_handle_t *d_zhp = NULL;
> 2145 2146 ZFS_CLOSE(zhp);
> 2147 2148 if ((d_zhp = zfs_open(g_zfs, clone_ds,
> ZFS_TYPE_FILESYSTEM))
> 2149 != NULL) {
> (void) zfs_destroy(d_zhp);
> ZFS_CLOSE(d_zhp);
> 2150 2151 }
> 2155 return (err);
> 2156 }
>
> This is logically equivalent to the previous but doesn't have the same
> 'backwardness' to it.
I think this is fine for now. What I was originally worried about was the fact
that if the zfs_destroy fails we would not catch that error and we could
silently leave the clone dataset hanging around. However I think fixing that so
we provide some notice that the clean-up after the original failure was also
not
successful is really more something that should be addressed in bug 9941 and
the
BE management observability project.
All this being said, I'm fine with the changes now.
-evan
>>
>> be_mount.c:
>> ===========
>> lines 238 and 395 ret should also be set to BE_SUCCESS
> Fixed.
>>
>> 1441 set err = BE_SUCCESS?
> Yes, done.
>>
>>
>> Other than these I don't see anything that hasn't already been mentioned.
>>
>> -evan
> New webrev will be posted after I've:
> * changed relevant "== 0" statements to "== BE_SUCCESS"
> * reviewed instances of err, errno, ret and zret for consistent usage
> * run the DIY libbe test against the changes
>
> - Keith
