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

WIP: Add a test for sss_nss_getgrouplist_timeout and fix invalidating the initgroups cache #558

Open
wants to merge 2 commits into
base: master
from

Conversation

@jhrozek
Copy link
Contributor

jhrozek commented Apr 24, 2018

This is a WIP on adding tests for the sss_nss_ex interface. I covered only the sss_nss_getgrouplist_timeout function so far.

I'm submitting the PR already in this state to get some feedback if this
coverage is enough and the other functions can be covered similarly or
if there is some issue with this approach.

Also, I found a bug in invalidating the initgroups memory cache, that's
the first of the two patches. Here I'm really not sure if the fix is even
how the issue should be fixed, so I just hacked something up, even without
allocation checks etc.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

jhrozek commented Apr 24, 2018

btw a hopefully better description of the invalidating memcache issue is in https://pagure.io/SSSD/sssd/issue/3718

@pbrezina

This comment has been minimized.

Copy link
Member

pbrezina commented Apr 27, 2018

Test looks good, but I will need better explanation of the issue in the first patch. Even pagure ticket is not verbose enough for me :-)

@sumit-bose

This comment has been minimized.

Copy link
Contributor

sumit-bose commented Apr 27, 2018

@pbrezina, maybe https://docs.pagure.org/SSSD.sssd/developers/mmap_cache_1.15.html#the-initgr-data can help to explain the peculiars of the name handling of the memory cache with initgroups requests.

@jhrozek jhrozek force-pushed the jhrozek:test_client_api branch from 094b179 to e9f7d71 Jan 20, 2019
@sumit-bose

This comment has been minimized.

Copy link
Contributor

sumit-bose commented Jan 21, 2019

retest this, please

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

jhrozek commented Jan 21, 2019

Just to explain why I pushed a new version: I didn't actually do anything new in this branch, but yesterday for some reason I don't understand all my partitions switched to read-only mode while on the train so I force-pushed my local git repo to github to have the most current snapshot in case my disk was about to die...and I didn't realize this would also propagate to the PRs.

@jhrozek jhrozek force-pushed the jhrozek:test_client_api branch from e9f7d71 to 0e7f363 Sep 4, 2019
@jhrozek

This comment has been minimized.

Copy link
Contributor Author

jhrozek commented Sep 4, 2019

Rebased per @pbrezina 's request

@pbrezina pbrezina self-assigned this Oct 31, 2019
@pbrezina

This comment has been minimized.

Copy link
Member

pbrezina commented Nov 4, 2019

Some of the tests fails. @jhrozek Is this still a wip? Should I take it over?

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

jhrozek commented Nov 4, 2019

Maybe? I don't plan on working on this, so do whatever you like..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.