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.

Sign up for GitHub

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

Jump to bottom

sss_ptr_hash: internal refactoring #977

Closed
wants to merge 7 commits into from
Closed

sss_ptr_hash: internal refactoring #977

wants to merge 7 commits into from

Conversation

alexey-tikhonov
Copy link
Member

sss_ptr_hash code was refactored:
- got rid of a "spy" to make logic cleaner
- table got destructor to wipe its content
- described some usage limitation in the documentation

Plus couple of minor issues were fixed and unit test added.

Resolves: https://pagure.io/SSSD/sssd/issue/4135

@alexey-tikhonov
Copy link
Member Author

Remark: scratch build was verified with both https://bugzilla.redhat.com/show_bug.cgi?id=1783190 and https://bugzilla.redhat.com/show_bug.cgi?id=1792331

@alexey-tikhonov
Copy link
Member Author

Remark: scratch build was verified with both https://bugzilla.redhat.com/show_bug.cgi?id=1783190 and https://bugzilla.redhat.com/show_bug.cgi?id=1792331

Also verified with https://bugzilla.redhat.com/show_bug.cgi?id=1796044 and https://bugzilla.redhat.com/show_bug.cgi?id=1793303

@pbrezina pbrezina self-assigned this Jan 31, 2020
@pbrezina
Copy link
Member

Thank you. It looks good to me. I have just some suggestions about deleting items that might simplify the code more...

The basic idea is to move hash_delete into sss_ptr_hash_value_destructor. Would it be possible?

Then you could do the following changes:

  • sss_ptr_hash_delete use lookup -> free(value) / calls hash_delete from cb -> free(payload)
  • sss_ptr_hash_delete_all - use hash_values instead of hash_keys and iterate over values and just call talloc_free(values)
  • sss_ptr_hash_delete_cb - talloc_steal(NULL, value) could be moved to sss_ptr_hash_value_destructor

pbrezina
pbrezina reviewed Jan 31, 2020
View reviewed changes
src/util/sss_ptr_hash.c Outdated Show resolved Hide resolved
@alexey-tikhonov
Copy link
Member Author

Thank you. It looks good to me. I have just some suggestions about deleting items that might simplify the code more...

Thank you for the feedback.

The basic idea is to move hash_delete into sss_ptr_hash_value_destructor. Would it be possible?

Then you could do the following changes:

* `sss_ptr_hash_delete` use lookup -> free(value) / calls hash_delete from cb -> free(payload)

* `sss_ptr_hash_delete_all` - use `hash_values` instead of `hash_keys` and iterate over values and just call `talloc_free(values)`

* `sss_ptr_hash_delete_cb` - `talloc_steal(NULL, value)` could be moved to `sss_ptr_hash_value_destructor`

Do I understand correctly that idea is to assure that the only possible execution chain to get into sss_ptr_hash_delete_cb() is:
talloc_free(sss_ptr_hash_value) -> sss_ptr_hash_value_destructor() -> hash_delete()
(so that sss_ptr_hash_delete_cb() doesn't need to check if execution is/is not in talloc_free(sss_ptr_hash_value) context)?

@pbrezina
Copy link
Member

pbrezina commented Feb 3, 2020

Yes.

@alexey-tikhonov
sbus_server: stylistic rename
887719d 
Renamed sbus_server_name_remove_from_table() to
sbus_server_name_remove_from_table_cb() to keep naming consistent
with other functions used as `hash_delete_callback` argument of
sss_ptr_hash_create()
@alexey-tikhonov
sss_ptr_hash: don't keep empty sss_ptr_hash_delete_data
6514a1d 
There is no need to allocate memory for `sss_ptr_hash_delete_data`
if table user doesn't provide custom delete callback.
@alexey-tikhonov
sss_ptr_hash: sss_ptr_hash_delete fix/optimization
d7f8742 
 - no reason to skip hash_delete() just because sss_ptr_hash_lookup_internal()
failed
 - avoid excessive lookup if it is not required to free payload
@alexey-tikhonov
sss_ptr_hash: removed redundant check
d84311e 
`sss_ptr_hash_check_type()` call would take care of this case.
@alexey-tikhonov
sss_ptr_hash: fixed memory leak
e669799 
In case `override` check was failed in _sss_ptr_hash_add()
`value` was leaking.
Fixed to do `override` check before value allocation.
@alexey-tikhonov
Copy link
Member Author

Do I understand correctly that idea is to assure that the only possible execution chain to get into sss_ptr_hash_delete_cb() is:
talloc_free(sss_ptr_hash_value) -> sss_ptr_hash_value_destructor() -> hash_delete()
(so that sss_ptr_hash_delete_cb() doesn't need to check if execution is/is not in talloc_free(sss_ptr_hash_value) context)?

Well, this is not that straightforward. We still could get to sss_ptr_hash_delete_cb() via explicit hash_delete() or implicitly via table d-tor -> hash_delete()...
Of course it was possible to detect and handle this keeping/using being_destructed flag but IMO this would defeat purpose of proposed improvements. So I changed implementation of table d-tor and documented it is not allowed to call hash_delete() explicitly...
Overall I think I like this version better.
I kept all the patches separately to easier review but probably it makes sense to squash it into one.
Some downstream tests didn't finish yet, but I think new version is ready for review.

@alexey-tikhonov
Copy link
Member Author

Downstream tests passed.

pbrezina
pbrezina requested changes Feb 14, 2020
View reviewed changes
src/util/sss_ptr_hash.c Show resolved Hide resolved
src/util/sss_ptr_hash.c Outdated Show resolved Hide resolved
src/util/sss_ptr_hash.c Outdated Show resolved Hide resolved
@pbrezina
Copy link
Member

Thank you. I have few nitpicks, but otherwise I am ready to accept these changes.

@alexey-tikhonov
sss_ptr_hash: internal refactoring
ebe704b 
sss_ptr_hash code was refactored:
 - got rid of a "spy" to make logic cleaner
 - table got destructor to wipe its content
 - described some usage limitation in the documentation

And resolves: https://pagure.io/SSSD/sssd/issue/4135
@alexey-tikhonov
TESTS: added sss_ptr_hash unit test
2ee3943
@alexey-tikhonov
Copy link
Member Author

I have few nitpicks, but otherwise I am ready to accept these changes.

I fixed two nitpicks and commented another.

Please let me know if you still (after reading my comment) think destructor should return -1 in case of hash_delete() fail. I actually do not have very strong preference here. Failure of hash_delete() means memory is already corrupted/inconsistent and recovery is hardly possible anyway.

@pbrezina
Copy link
Member

Ok, so looking at hash_delete definition it is safe to assume that the entry is either deleted or not found.

@pbrezina
Copy link
Member

Ack. Thank you.

@pbrezina
Copy link
Member

Failure is not related.

@pbrezina pbrezina added the Ready to push Ready to push label Feb 17, 2020
@pbrezina
Copy link
Member

  • master
    • 88b23bf - TESTS: added sss_ptr_hash unit test
    • 0bb1289 - sss_ptr_hash: internal refactoring
    • 4bc0c2c - sss_ptr_hash: fixed memory leak
    • 8cc2ce4 - sss_ptr_hash: removed redundant check
    • d0eb880 - sss_ptr_hash: sss_ptr_hash_delete fix/optimization
    • adc7730 - sss_ptr_hash: don't keep empty sss_ptr_hash_delete_data
    • faa5dbf - sbus_server: stylistic rename

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Feb 17, 2020
@pbrezina pbrezina closed this Feb 17, 2020
@pbrezina pbrezina mentioned this pull request Apr 28, 2020
Closed
@sssd-bot sssd-bot mentioned this pull request May 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbrezina pbrezina pbrezina approved these changes

Assignees

@pbrezina pbrezina

Labels
Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexey-tikhonov @pbrezina