-
Notifications
You must be signed in to change notification settings - Fork 22
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
Cache location config #120
Cache location config #120
Conversation
There has been desire to create a method by which users can configure where to store the key cache without setting the env var `$XDG_CACHE_HOME` or relying on the user to have a home dir. This is especially relevant in cases where the library is embedded in a daemon (e.g. XRootD or HTCondor) that may have a preferencial key cache location. This commit adds a familiar `config_set_str` and `config_get_str` API for passing a new cache home to the library through the use of the `keycache.cache_home` key. As with `config_set_int` and `config_get_int`, these new functions can be easily updated in the future as the project develops and new configuration keys are desired. This would entail adding a new check for the desired key and the logic to handle any values associated with that key.
Getting rid of a few commented blurbs, cleaning up a few extraneous spaces, etc.
Suddenly I have opinions about allowing the robot to fix, and not just complain about, linter issues! I would like to make this thread safe. Can you please replace the the storage of this information with a std::shared_ptrstd::string? |
src/scitokens_internal.cpp
Outdated
std::string component; | ||
|
||
while (std::getline(ss, component, '/')) { | ||
pathComponents.push_back(component); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone provides a path of ///foo//bar
? May need an if (!component.empty())
here to avoid inserting unnecessary items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You caught my sinful shortcut. Since ultimately this output only gets passed to mkdir, the POSIX standard should suppress the extra slashes (so that the cache would be dumped at /foo/bar
even if it was specified as ///foo//bar
). But I agree, I should add the check in case this function ever gets used in another spot where someone might not know about that peculiarity.
src/scitokens_internal.cpp
Outdated
std::vector<std::string> pathComponents = path_split(dir_path); | ||
for (const auto &component : pathComponents) { | ||
currentLevel += "/" + component; | ||
if (!check_dir(currentLevel)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't check the dir first; just create it and inspect the mkdir
return code (if it's EEXIST
, then the directory already exists and move on).
For error message creation, it would be better to have an output variable that's the error message so you can say precisely the file operation that failed. The caller of this function currently has something generic that won't be useful for debugging.
The previous code worked when configured to use a cache home like `///foo//bar`, but it wasn't set up to handle this situation robustly -- small future changes could have easily broken the functionality, so I modified the path parsing and directory checking functions to better handle this. I also changed the way error messages are propagated by these new config options to make sure that any errors thrown when calling `mkdir()` make it to the config API's err_msg output variable. This should make debugging a bit easier in any cases where someone unwittingly tries to plop their cache someplace where they don't have read/write access.
Hmm, any ideas what might be causing the |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
You say that, but now it looks good. Are you happy with this pull request now? |
@jhiemstrawisc - can you clean up the remaining linter items? Once that's done, I think we're ready to go. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@bbockelm Okay, latest commit made the linter happy, and all the tests are passing. |
The symbols config_set_str() and friends seem far too generic for a public library with C bindings. |
Unfortunately, the choice of names for the config_set_blah() family of APIs is quite generic and poses the potential to cause C symbol collisions when SciTokens is used by large projects that dynamically load lots of libraries (a la HTCondor). A reasonable plan to address this, since those APIs are in use and changing their names would be API-breaking, seems to be adding a second set of prefixed APIs, with the intention of deprecating the old functions at the next major release.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
With the new config functions prefixed with |
…_set_blah() To cut down on code duplication, the config_set_blah() family of APIs have been changed to immediately call the scitokens-prefixed family of APIs under the hood.
Per Jaime's comment, since we plan to deprecate the `config_set_blah()` family of functions in favor of the prefixed versions, I've updated the internal API documentation to point people toward the preferred APIs.
Anything else on this, or are we ready to wrap things up? |
Nothing from my end. |
I wonder how fast we can depreciate the config_* functions. Is HTCondor already using them? I know xrootd isn't. If no one is using them, can we just remove them already? |
Good point. The config_set_str() and config_get_str() were never released. Why release a deprecated API? HTCondor does not use config_set_int() or config_get_int(). From HTCondor's point of view, it could be removed immediately. |
I just noticed that the other functions start with |
Oh, I thought I had heard that the older |
…oken It looks like the config_set/get_blah APIs weren't actually in use and hadn't been released, so instead of keeping them around, we decided to just delete them. Also, the `scitokens`-prefixed versions of these APIs were changed to be prefixed by `scitoken` to match other functions in the library.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I mistakenly removed the `config_set/get_int` APIs when removing `config_set/get_str`.
I think that Brian B wanted to have the C++ deprecated decorator (not just a comment). https://en.cppreference.com/w/cpp/language/attributes/deprecated |
Since scitokens.h contains C declarations, I don't think the C++ deprecated decorator is appropriate (or even allowed). |
Brian B said to worry about any deprecated decorators in another PR at another time. I think he's looking to have this PR wrapped up expediently. It looks like the deprecated decorator for C is supported after C23: https://en.cppreference.com/w/c/language/attributes/deprecated, whereas the deprecated decorator for C++ has been supported since C++14. |
Yeah - unfortunately, we’ll need to wrap the deprecated stuff in some macro as it’s only recently standardized in C… hence punting it to a new PR. |
Since we aren't adding the decorator for deprecation warnings in this PR, is there anything else needed to close this? |
I think that it is good to go. |
Nothing from me. |
Is Otherwise, I think we're in good shape here. |
Let's keep |
See Issue #118
There has been desire to create a method by which users can configure
where to store the key cache without setting the env var
$XDG_CACHE_HOME
or relying on the user to have a home dir. This is especially relevant
in cases where the library is embedded in a daemon (e.g. XRootD or HTCondor)
that may have a preferencial key cache location. This commit adds a familiar
config_set_str
andconfig_get_str
API for passing a new cache home tothe library through the use of the
keycache.cache_home
key.As with
config_set_int
andconfig_get_int
, these new functions can beeasily updated in the future as the project develops and new
configuration keys are desired. This would entail adding a new check for
the desired key and the logic to handle any values associated with that key.
Some comments/questions:
a garbage path is provided to the configuration API? I made the
assumption that we weren't, since there were no checks on what
was supplied by
XDG_CACHE_HOME
. Currently, the implementationtries to create whatever directory is passed through the API if it doesn't
exist, but this could break if the user has insufficient permissions to
write to that location, or the path contains invalid chars (and probably
some other things I'm not thinking of).
class. Would we rather those be moved to a new class dedicated to this
type of functionality? I noticed in testing that if
XDG_CACHE_HOME
isset to a directory that doesn't already exist, the current code will fail if
any parent dirs need to be created. The new config API attempts to create
those parent dirs, so maybe a stub class to handle these types of things
would be useful.