Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverity: fix REVERSE_INULL for pevent->inst #11

Closed
wants to merge 1 commit into from

Conversation

tkrizek
Copy link
Contributor

@tkrizek tkrizek commented Mar 29, 2017

With the DynDB API changes, the ldap instance is acquired
differently. Previously, obtaining the instance could fail when
LDAP was disconnecting, thus the NULL check was necessary in the
cleanup part.

Now, inst is obtained directly from the API. I'm not sure what is
the exact behaviour in edge cases such as LDAP disconnecting, so
I perform the NULL check a bit earlier, just to be safe.

@tkrizek
Copy link
Contributor Author

tkrizek commented Mar 29, 2017

@pemensik Hi, could you take a quick look at this change?

I ran coverity and the issues were fixed.

It might also be possible to remove the REQUIRE, but since I'm not sure whether inst is always non null in the new dyndb workflow, I added the check just to be sure.

@pemensik
Copy link
Contributor

Hi Tomáš,

I did not find any place which could result with inst == NULL. ldap_sync_prepare contains proper REQUIRE(inst != null) and all other function uses its result stored in ls_private. I think null check does not harm anything. It is used in BIND to catch unexpected problems after code changes. I would suggest to place one require at the beginning of syncrepl_update function also.

Overall I think it should be merged.

With the DynDB API changes, the ldap instance is acquired
differently. Previously, obtaining the instance could fail when
LDAP was disconnecting, thus the NULL check was necessary in the
cleanup part.

Now, inst is obtained directly from the API. I'm not sure what is
the exact behaviour in edge cases such as LDAP disconnecting, so
I perform the NULL check a bit earlier, just to be safe.
@tkrizek tkrizek force-pushed the fix-reverse-inull-coverity branch from e5c2989 to 07ed3eb Compare April 3, 2017 15:26
@tkrizek
Copy link
Contributor Author

tkrizek commented Apr 3, 2017

Thanks for the review, Petr!

I added the check to syncrepl_update as well.

@tkrizek tkrizek added the ack label Apr 6, 2017
@tkrizek
Copy link
Contributor Author

tkrizek commented Apr 6, 2017

master:

@tkrizek tkrizek closed this Apr 6, 2017
@tkrizek tkrizek added the pushed label Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants