Ethan Quach wrote:
>
> Keith Mitchell wrote:
>> ... a webrev has been posted at
>> http://cr.opensolaris.org/~kemitche/3837b/
>
> Comments on the updated webrev...
>
>
> be_activate.c
> --------------------
> 187,198,216,225,231,238 - I know this code was already there
> but ..., unless a variable is of type boolean, its generally not good
> practice to use it that way. In this particular case, it especially
> dangerous because the code here assumes BE_SUCCESS == 0.
> Please change these to "if (ret != BE_SUCCESS)", or better yet
> combine the pairs of lines into one each.
Fixed.
>
> 615 - (Again, not your changes, but ..) Here's another
> assumption that BE_SUCCESS == 0. Please change this to
> "if (err == BE_SUCCESS)"
Fixed.
>
> 940 - Same comment. (And youch, that should have been
> a "==" instead of a "=" :-D )
Fixed as well. In both cases also updated use of ret & err.
>
> 1080 - Since be_promote_ds_callback() explicitly returns
> 0 for success, this should stay compared to 0 to check against
> success.
Reverted.
>
>
> be_create.c
> ------------------
> 471 - The setting of ret here doesn't do anything. It gets
> masked with something else before the function returns in
> all code paths anyway, so can you change this line to
>
> if (be_get_uuid(...) != BE_SUCCESS)
>
Ok.
>
>
> 977 - Compare against 0 here instead.
Fixed.
>
> 1007 - Same comment as for 471.
Fixed.
>
> 1245,1298,1647,1745 - change this to "be_errno_t - Failure"
Ok.
>
> 1401,1437,1982 - Compare against 0 here instead.
Ok.
>
> 2176 - line needs to be reverted.
Done.
>
>
> be_mount.c
> ------------------
> 529, 592, 2135, 2235, 2327, 2407 - change this to
> "be_errno_t - Failure"
Done.
>
> 537 - just a nit - err is used only to store errno in here, so
> it doesn't really need to be initialized to BE_SUCCESS.
Fixed. My initial approach to this bug was much a bit naive...
>
> 894 - Same comment as previous.
Fixed.
>
> 1492 - initialize to 0 instead.
> 1515 - return an explicit 0.
Thought I'd reverted all those... fixed now.
>
> 2441 - This should be returning 'ret' here. Can you fix
> this with your changes.
Sure.
>
> 2511 - this needs to be reverted.
Done.
>
>
> be_snapshot.c
> ----------------------
> 319-320 - revert these lines.
> 338 - revert this line.
Reverted
>
> 368 - typo "be_errno_t"
Fixed.
>
>
> be_utils.c
> ---------------
> 554,1012,1173,1399,1692 - Please change these to
> "if (<var> == BE_SUCCESS)"
Done.
>
>
> At lines 3502-3503, we're simply returning an errno, and
> for the success case errno == 0, so saying we're returning
> BE_SUCCESS for success at 3429 means we're making the
> assumption BE_SUCCESS == 0.
>
> I think we should revert lines 3429 and 3439; and then just
> change 3477-3478 to:
>
> if (_be_list(NULL, &be_nodes) == BE_SUCCESS) {
>
>
> The setting of err there gets masked later on anyway so
> it doesn't seem like we need to set it here.
The purpose of bug 3837 is to have consistent return codes throughout
libbe. While the *_callback functions need to return 0 for success as
previously discussed, the remaining functions should return BE_SUCCESS
for successful cases. It's not the most elegant option, but explicitly
returning BE_SUCCESS when err == 0 seems more appropriate:
*menu_fp = fopen(menu_file, mode);
err = errno;
if (err != 0)
return (err);
return (BE_SUCCESS);
What do you think?
>
>
> be_zones.c
> -----------------
> 105, 181, 328 - change this to "be_errno_t - Failure"
Done.
>
>
>
> thanks,
> -ethan
>
