URL: https://github.com/SSSD/sssd/pull/977 Author: alexey-tikhonov Title: #977: sss_ptr_hash: internal refactoring Action: opened
PR body: """ 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
"""
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/977/head:pr977 git checkout pr977
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """ 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 """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-579794511
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """
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 """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-579860088
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ 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`
"""
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-580706435
URL: https://github.com/SSSD/sssd/pull/977 Author: alexey-tikhonov Title: #977: sss_ptr_hash: internal refactoring Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/977/head:pr977 git checkout pr977
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """
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)?
"""
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-580773585
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ Yes. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-581346807
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/977 Author: alexey-tikhonov Title: #977: sss_ptr_hash: internal refactoring Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/977/head:pr977 git checkout pr977
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """
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.
"""
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-585612874
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """ Downstream tests passed. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-585774357
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ Thank you. I have few nitpicks, but otherwise I am ready to accept these changes. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586216541
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Author: alexey-tikhonov Title: #977: sss_ptr_hash: internal refactoring Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/977/head:pr977 git checkout pr977
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
alexey-tikhonov commented: """
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.
"""
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586261265
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ Ok, so looking at [hash_delete definition](https://pagure.io/SSSD/ding-libs/blob/master/f/dhash/dhash.c#_1064) it is safe to assume that the entry is either deleted or not found. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586263722
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ Ack. Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586264129
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ Failure is not related. """
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586922172
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
pbrezina commented: """ * `master` * 88b23bf50dd1c12413f3314639de2c3909bd9098 - TESTS: added sss_ptr_hash unit test * 0bb1289252eec972ea26721a92adc7db47383f76 - sss_ptr_hash: internal refactoring * 4bc0c2c7833dd643fc1137daf6519670c05c3736 - sss_ptr_hash: fixed memory leak * 8cc2ce4e9060a71d441a377008fb2f567baa5d92 - sss_ptr_hash: removed redundant check * d0eb88089b059bfe2da3bd1a3797b89d69119c29 - sss_ptr_hash: sss_ptr_hash_delete fix/optimization * adc7730a4e1b9721c93863a1b283457e9c02a3c5 - sss_ptr_hash: don't keep empty sss_ptr_hash_delete_data * faa5dbf6f716bd4ac0a3020a28a1ee6fbf74654a - sbus_server: stylistic rename
"""
See the full comment at https://github.com/SSSD/sssd/pull/977#issuecomment-586927612
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/977 Title: #977: sss_ptr_hash: internal refactoring
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/977 Author: alexey-tikhonov Title: #977: sss_ptr_hash: internal refactoring Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/977/head:pr977 git checkout pr977
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
pbrezina