Evan,
be_create.c
-----------
182 - Can we create a more specific error code for this one.
I think I see a BE_ERR_CREATDS used in another place for
this.
219 - I don't see how this relates to a ZFS error. BE_ERR_INVAL
seems more appropriate.
427,684 - These should return BE_ERR_NOENT instead.
443 - "%s\n"
462 - Can we return something like BE_ERR_MOUNTED here instead?
756 - Should be BE_ERR_INVAL instead.
873 - We're actually creating the snapshot here, so its probably
more appropriate to return the error code returned from
_be_create_snapshot(). Either that or return something
with more local meaning like BE_ERR_SNAPSHOT
1065,1076 - Should these be " ... != BE_SUCCESS" ?
1450,1578 - Should these now be be_errno_t
1469,1471 - Should we just return the error code from
be_prep_clone_send_fs() ?
be_mount.c
----------
306, 380, 565, 873, 1526 - Why not return the error code from
zfs_iter_filesystems() since these callback functions have
been changed to return error codes?
444 - BE_ERR_NOENT
542 - Can we return a better code for this case? Something like
BE_ERR_INVAL would be more descriptive in my opinion since
we're essentially validating the BE someone is trying to
unmount.
694 - setting ret here would potentially hide some other error
since line 664. Its possible for the _be_unmount() to
fail in addition to some other error, but I think reporting
the other error would be more valuable. So perhaps, this
should be:
if (ret == 0)
ret = err;
986 - Not sure, but could we come up with a more detailed code
for this case?
993 - BE_ERR_ZFS here instead?
1001 - same comment as 694.
1093 - add_to_fs_list() - unless we're actually passing its return
code along (which we're currently not, see lines 647 and 1060),
I don't really see the need to change what this function is
returning.
1148 and 1217 - perhaps we *should* collect and pass along its
return code? If agreed, this can be addressed in a separate
defect.
1403 - get_mountpoint_from_vfstab() - same comment as 1093.
be_rename.c
-----------
117 - zfs didn't error here, so doing this gets us who knows what.
We should return BE_ERR_NOENT instead (like the other places
where we do this same call)
be_snapshot.c
-------------
347 - We should return the error code from the call to
be_rollback_callback() at line 342.
411,590 - BE_ERR_NOENT
665 - We've defined BE_ERR_NOENT in libbe.h as "No such BE",
but we're using this error code for
"No such <other things>", like here when a snapshot doesn't
exist. Should we define more detailed BE_ERR_<blah>_NOENT
error codes for the different uses? e.g.
BE_ERR_POOL_NOENT
BE_ERR_BE_NOENT
BE_ERR_SS_NOENT
... and leave the BE_ERR_NOENT for the generic cases (and
I guess for when we are converting from zfs errors or errno
errors as well)
672 - Return the return code from the call to zfs_iter_filesystems()
at 669 instead.
be_utils.c
----------
Throughout this file, fopen() and open() failures seem to be treated
inconsistently. Sometimes ret is set to BE_ERR_OPEN, and in others,
ret is set to errno_to_be_err(err). Can we be more consistent with
these?
1462 - same comment as I had previously for be_mount.c:694
1872-1876 - Should we use a static flag in this function so that the
env is only checked once?
Or at a minimum, this chunk could be moved to be an else clause at 1882.
1899 - These zfs error codes seem like ones we might want to capture:
EZFS_MOUNTFAILED,
EZFS_UMOUNTFAILED,
EZFS_POOLUNAVAIL,
EZFS_RESILVERING,
EZFS_DSREADONLY,
EZFS_BADTYPE,
EZFS_PROPNONINHERIT,
EZFS_PROPTYPE,
EZFS_PROPREADONLY,
-ethan
Evan Layton wrote:
> This fix adds the use of error codes throughout libbe. All functions
> should now return the errors codes defined in libbe.h.
>
> I've also added the use of an environment variable (BE_PRINT_ERR) that
> enables printing the error output in the library without having to
> compile in changes to enable this printing.
>
> Defect:
> ------
> http://defect.opensolaris.org/bz/show_bug.cgi?id=1817
>
> Webrev:
> ------
> http://cr.opensolaris.org/~evanl/1817/
>
> Thanks,
> -evan
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
