Just my ancient 2 cents. If you need to do the test every time you call the function then it should be in the function.
On Thu, Oct 12, 2017 at 3:06 PM, Matthew Ahrens <[email protected]> wrote: > > > On Wed, Oct 11, 2017 at 10:10 PM, Tobin C. Harding <[email protected]> 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> ------------------------------------------ openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9490ee2cf4250913-M0031d05e2502d1bed6397aab Powered by Topicbox: https://topicbox.com
