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.
615 - (Again, not your changes, but ..) Here's another
assumption that BE_SUCCESS == 0. Please change this to
"if (err == BE_SUCCESS)"
940 - Same comment. (And youch, that should have been
a "==" instead of a "=" :-D )
1080 - Since be_promote_ds_callback() explicitly returns
0 for success, this should stay compared to 0 to check against
success.
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)
977 - Compare against 0 here instead.
1007 - Same comment as for 471.
1245,1298,1647,1745 - change this to "be_errno_t - Failure"
1401,1437,1982 - Compare against 0 here instead.
2176 - line needs to be reverted.
be_mount.c
------------------
529, 592, 2135, 2235, 2327, 2407 - change this to
"be_errno_t - Failure"
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.
894 - Same comment as previous.
1492 - initialize to 0 instead.
1515 - return an explicit 0.
2441 - This should be returning 'ret' here. Can you fix
this with your changes.
2511 - this needs to be reverted.
be_snapshot.c
----------------------
319-320 - revert these lines.
338 - revert this line.
368 - typo "be_errno_t"
be_utils.c
---------------
554,1012,1173,1399,1692 - Please change these to
"if (<var> == BE_SUCCESS)"
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.
be_zones.c
-----------------
105, 181, 328 - change this to "be_errno_t - Failure"
thanks,
-ethan
