On Wed, Oct 11, 2017 at 10:10 PM, Tobin C. Harding <m...@tobin.cc> wrote:
> Hi,
>
> Coverity emits warnings in a number of places about code like this
>
> errout:
> if (zhp != NULL)
> zfs_close(zhp);
>
> One example warning is
>
> CID 166345 (#1 of 1): Dereference before null check
> (REVERSE_INULL)check_after_deref: Null-checking
> zhp suggests that it may be null, but it has already been dereferenced on
> all paths leading to the
> check
>
>
> A previous PR was merged that changed the code to look like this
>
>
> The warning is that
>
> errout:
> ASSERT3P(zhp, !=, NULL);
> zfs_close(zhp);
>
>
> The question is; is this just changing the code to satisfy coverity? If
> so, that seems like a bad
> idea.
>
Assuming the change is valid, I think it's a good change to make in
general, regardless of coverity.
>
> Would ZFS be open to moving the null check into zfs_close()? (extra white
> space for clarity).
>
> /*
> * Release a ZFS handle. Nothing to do but free the associated memory.
> */
> void
> zfs_close(zfs_handle_t *zhp)
> {
>
> + if (zhp == NULL)
> + return;
>
> if (zhp->zfs_mntopts)
> free(zhp->zfs_mntopts);
> nvlist_free(zhp->zfs_props);
> nvlist_free(zhp->zfs_user_props);
> nvlist_free(zhp->zfs_recvd_props);
> free(zhp);
> }
>
>
> Does this improve the code?
> Does it quiet coverity?
>
We could do that, but I think that if possible, changing the calling code
is cleaner. That said, I don't think it's a huge deal either way.
>
> Long email for such a trivial question ;)
>
> Last question; is this the correct channel to open general questions
> relating to the code base or do
> you guys prefer issues on GitHub?
>
This is a fine place. We haven't used GitHub issues much in the past (we
do use the pull requests heavily though).
--matt
>
> thanks,
> Tobin.
------------------------------------------
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T9490ee2cf4250913-Med763d805151a2c80bb3b0f5
Powered by Topicbox: https://topicbox.com
