Hi Ethan,
Thanks for the comments!
I've updated the webrev and included my responses below.
Thanks again!
-evan
Ethan Quach wrote:
> 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.
>
fixed.
> 219 - I don't see how this relates to a ZFS error. BE_ERR_INVAL
> seems more appropriate.
You're correct it should have been BE_ERR_INVAL. I'm not sure how it ended up
with BE_ERR_ZFS.
>
> 427,684 - These should return BE_ERR_NOENT instead.
definitely.
>
> 443 - "%s\n"
fixed.
>
> 462 - Can we return something like BE_ERR_MOUNTED here instead?
Yes that is more correct, fixed.
>
> 756 - Should be BE_ERR_INVAL instead.
Yes it should.
>
> 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
I changed this to return the error code from _be_create_snapshot().
>
> 1065,1076 - Should these be " ... != BE_SUCCESS" ?
makes it more readable so yes.
>
> 1450,1578 - Should these now be be_errno_t
fixed
>
> 1469,1471 - Should we just return the error code from
> be_prep_clone_send_fs() ?
Yes we should, fixed.
>
>
> 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?
fixed
>
> 444 - BE_ERR_NOENT
fixed
>
> 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.
That makes good sense. fixed
>
> 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;
Good point. I've made this change here but there are a couple
places in the rest of the code that do this same thing. I'll
make the same change in these places as well.
>
> 986 - Not sure, but could we come up with a more detailed code
> for this case?
BE_ERR_NOTMOUNTED ?
>
> 993 - BE_ERR_ZFS here instead?
fixed
>
> 1001 - same comment as 694.
fixed
>
> 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.
You're correct that we don't really need to pass these back at this
point. I'll change these back. We can always change this function
to pass errors back if we need to in the future.
>
> 1148 and 1217 - perhaps we *should* collect and pass along its
> return code? If agreed, this can be addressed in a separate
> defect.
I agree. I'll create a new bug for this.
>
> 1403 - get_mountpoint_from_vfstab() - same comment as 1093.
fixed
>
>
> 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)
fixed
>
>
> be_snapshot.c
> -------------
>
> 347 - We should return the error code from the call to
> be_rollback_callback() at line 342.
fixed
>
> 411,590 - BE_ERR_NOENT
fixed
>
> 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)
Yep I like this idea. I'm making these changes throughout the library.
>
>
>
> 672 - Return the return code from the call to zfs_iter_filesystems()
> at 669 instead.
fixed
>
>
> 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?
Crap, I thought I had fixed all these. I'll run through again and make
sure this is consistent throughout the library.
>
> 1462 - same comment as I had previously for be_mount.c:694
fixed
>
> 1872-1876 - Should we use a static flag in this function so that the
> env is only checked once?
I wanted to be able to turn it on and off at any point even if the
library were in the middle of processing. By checking the env every
time we call be_print_err() we can turn this on and off at will.
> Or at a minimum, this chunk could be moved to be an else clause at 1882.
Do you mean instead of setting the do_print flag to true?
maybe something like this?
va_start(ap, prnt_str);
if (do_print) {
(void) vsprintf(buf, prnt_str, ap);
(void) fprintf(stderr, buf);
} else if ((env_buf = getenv("BE_PRINT_ERR")) != NULL) {
if (strcasecmp(env_buf, "true") == 0) {
(void) vsprintf(buf, prnt_str, ap);
(void) fprintf(stderr, buf);
}
}
>
> 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,
added these...
>
>
> -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
>
>
