URL:
https://github.com/SSSD/sssd/pull/5375
Title: #5375: kcm: improve performance for large ccaches
pbrezina commented:
"""
Ouch... This is really large!
Thank you for thorough review. You did an excellent job.
I have to re-read "kcm: store credentials list in hash table to
avoid cache lookups " and "sss_ptr_hash: fix double free for circular
dependencies" patches. Btw, don't you think it makes sense to factor out the
latter into stand alone PR? It would create additional hustle while backporting patches
downstream, but that looks like separate bugfix.
It is required for this patch set to work correctly, otherwise some use cases are broken
(`kvno` command will fail), so it needs to stay bound to this PR. However, as far as I can
tell, it is not needed by other uses of `sss_ptr_hash`.
I've left a number of comments inline. Of those I'd like to
highlight default hash table size and covscan error.
Thank you. Those that were fixed are marked as resolved.
Besides that my main and most important question is as follows:
Is it really worth the trouble to keep backward compatibility for the sake of existing
ccaches? That's quite a pile of code to support JSON format in kcm and encrypted
payload in "secdb"... We could simplify things a lot by dropping enc_type and
data_type entirely (perhaps alongside with secrets responder and even parts of
util/crypto).
I can imagine you have different view since you already put this effort in.
But is it at least something we could consider for a future release, when old format user
ccaches should be expired?
You are completely correct. It is mostly needed to keep compatibility with secrets
responder and we got smooth upgrade without the need to re-run kinit almost for free which
is nice. I'm definitely pro dropping secrets code but I didn't want to do this as
part of this PR.
Could you please explain claim "2)" in the
"sss_ptr_hash: fix double free for circular dependencies" patch?
Honestly, I can not. I already forgot the details and it is not directly reproducable. The
reproducer was to run `kvno` to obtain a service ticket. It called `GET_CRED_UUID_LIST`
several times on the same connection so with hit `kcm_creds_to_table()` multiple times on
non-empty table (which is perfectly valid case as the comment describes). This lead to the
first issue, when hash_delete(old) was called from hasn_enter() to overwrite existing
entry. I hit the second issue (crash in talloc) with few non-final patches for the first
issue so I can't tell you how to reproduce. There was some catch in it bound to the
fact that an entry is being overwritten. I can remove it from the description if you
want?
"""
See the full comment at
https://github.com/SSSD/sssd/pull/5375#issuecomment-734270404