On the face of it, it sounds reasonable, but one possible thing is that it may “mask” bugs caused by NULL pointers existing where they should not.
An equally valid approach, I think, is to use an assertion instead of a check. If we know that it *must* be NULL at that point in the code, then just “assert” it with a comment. On Wed, Oct 11, 2017 at 10:11 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. > > 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? > > 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? > > thanks, > Tobin. ------------------------------------------ openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9490ee2cf4250913-M45850485782a5da43a242d87 Powered by Topicbox: https://topicbox.com
