Evan Layton wrote:
> Jean McCormack wrote:
>> be_activate.c
>> line 606+
>> I'm concerned that err is declared a uint_t and then is returned.
>> Note that the function returns an int.
>> I also wonder about the code (you didn't modify it but deals with err
>> and errno) at lines 634-638.
>>
>
> I'm not sure how or why this ended up being unit_t but it should be an
> int.
> As for lines 634-638 we're capturing the errno and translating it into
> a BE error code.
Fixed.
>
>
>> be_create.c:
>> line 2404: This will be ok in this case but note that before i was
>> undefined and now you've initialized it to 0.
Reverted that.
>>
>> be_mount.c:
>> line 680: should that be comparing to != BE_SUCCESS?
Yes, I'm going back through and changing this and similar instances.
>>
>> be_rename.c:
>> line 139: I'm wondering how ret can be anything but 0 at this point?
>> Is the else dead code?
>
> ret should be getting set at line 134 but it looks like that got
> removed at some point. :-(
>
> it should look more like this:
> 134 if ((ret = zpool_iter(g_zfs, be_find_zpool_callback,
> &bt)) == 0) {
> 135 be_print_err(gettext("be_rename: failed to "
> 136 "find zpool for BE (%s)\n"), bt.obe_name);
> 137 be_zfs_fini();
> 138 return (BE_ERR_BE_NOENT);
> 139 } else if (ret < 0) {
> 140 be_print_err(gettext("be_rename: zpool_iter
> failed: %s\n"),
> 141 libzfs_error_description(g_zfs));
> 142 ret = zfs_err_to_be_err(g_zfs);
> 143 be_zfs_fini();
> 144 return (ret);
> 145 }
>
Fixed.
>
>>
>> be_utils.c
>> line 1114: Should you be comparing to BE_SUCCESS?
>> line 3296: same thing
>>
>> be_zones.c:
>> line 412: should you be comparing to BE_SUCCESS?
>>
>> You might want to look for the general case where you see something
>> like this:
>>
>> (ret = be_function_call()) != 0
>>
>> You might need to be comparing to BE_SUCCESS
Yes, I'll go back through and make that change.
>>
>> Jean
>>
=====
> Hi Keith,
>
> be_create.c:
> =============
> line 526 zret should probably be just ret and line 531 can probably be
> removed.
Done.
>
> 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.
>
> 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.
>
> 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
