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

FilterUtility: Replace some nested raw pointers by unique_ptr<>* #9537

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

yhabteab
Copy link
Member

No description provided.

@yhabteab yhabteab requested review from julianbrost and Al2Klimov and removed request for julianbrost October 12, 2022 14:57
lib/remote/filterutility.hpp Outdated Show resolved Hide resolved
lib/remote/filterutility.hpp Outdated Show resolved Hide resolved
lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from 7ec50d2 to 0efd86f Compare October 13, 2022 12:19
@cla-bot cla-bot bot added the cla/signed label Oct 13, 2022
@yhabteab yhabteab requested a review from Al2Klimov October 13, 2022 12:20
lib/remote/eventqueue.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from 0efd86f to 3434f48 Compare October 13, 2022 14:01
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riddle

➜  icinga2 git:(replace-some-raw-pointer-with-intrusive-ptr) curl -ksSu root:123456 -H 'Accept: application/json' -X PUT -d '{"attrs":{"check_command":"passive","enable_active_checks":false}}' https://127.0.0.1:5665/v1/objects/hosts/e;echo
curl: (52) Empty reply from server

➜  icinga2 git:(replace-some-raw-pointer-with-intrusive-ptr) curl -ksSu root:123456 -H 'Accept: application/json' -X PUT -d '{"attrs":{"check_command":"passive","enable_active_checks":false}}' https://127.0.0.1:5665/v1/objects/hosts/e;echo
curl: (7) Failed to connect to 127.0.0.1 port 5665 after 5 ms: Connection refused

➜  icinga2 git:(replace-some-raw-pointer-with-intrusive-ptr)

lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from 3434f48 to af0aa7d Compare October 13, 2022 15:02
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from af0aa7d to 17cc0a0 Compare October 13, 2022 15:28
@yhabteab yhabteab requested a review from Al2Klimov October 13, 2022 15:28
@julianbrost
Copy link
Contributor

Given that this PR is based on my suggestion from #9408 (comment), why did you chose intrusive_ptr over unique_ptr? (I think it should be perfectly doable with unique_ptr, only objectqueryhandler.cpp would need a slightly more elaborate change.)

@yhabteab
Copy link
Member Author

Given that this PR is based on my suggestion from #9408 (comment), why did you chose intrusive_ptr over unique_ptr?

We are constantly transferring the ownership of permissionFilter around in objectqueryhandler.cpp, how do you suppose that would look/work with unique_ptr<>? However, if you want some fricky code to workaround this, I can change it to unique_ptr<>, then you would have to keep moving it back and forth.

@julianbrost
Copy link
Contributor

However, if you want some fricky code to workaround this, I can change it to unique_ptr<>, then you would have to keep moving it back and forth.

There's a way around this: put the unique_ptr in the map, everything else can be a reference to it. Hint: insert returns an iterator to the inserted element, this is useful for initializing that reference.

@Al2Klimov
Copy link
Member

But keep in mind:

If rehashing occurs due to the insertion, all iterators are invalidated.

https://en.cppreference.com/w/cpp/container/unordered_map/insert

@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from 17cc0a0 to fa70b2f Compare October 17, 2022 14:29
@yhabteab yhabteab requested review from Al2Klimov and julianbrost and removed request for julianbrost and Al2Klimov October 17, 2022 14:31
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from fa70b2f to e0155c7 Compare October 17, 2022 14:32
@yhabteab yhabteab changed the title FilterUtility: Replace some nested raw pointers by our ::Ptr* FilterUtility: Replace some nested raw pointers by unique_ptr<>* Oct 17, 2022
lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
lib/remote/objectqueryhandler.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from e0155c7 to cb1e2b8 Compare October 17, 2022 17:52
@yhabteab yhabteab requested a review from julianbrost October 17, 2022 17:52
lib/remote/filterutility.cpp Outdated Show resolved Hide resolved
lib/remote/objectqueryhandler.cpp Show resolved Hide resolved
@yhabteab yhabteab force-pushed the replace-some-raw-pointer-with-intrusive-ptr branch from cb1e2b8 to c1f73fb Compare November 28, 2022 13:51
@yhabteab yhabteab requested a review from julianbrost November 28, 2022 13:52
@julianbrost julianbrost added this to the 2.14.0 milestone Nov 29, 2022
@Al2Klimov Al2Klimov merged commit ca32862 into master Dec 6, 2022
@icinga-probot icinga-probot bot deleted the replace-some-raw-pointer-with-intrusive-ptr branch December 6, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants