Conversation
ronen-fr
added
bug-fix
needs-quincy-backport
src/common/config_cacher.h
Outdated
| : conf(conf), | ||
| option_name(option_name) { | ||
| keys{option_name, nullptr}, | ||
| option_name(keys[0]) { |
There was a problem hiding this comment.
This assumes certain guarantees about the life-time of the passed option_name. It's nothing new but, if the config_cacher_t get rolled out broadlier, it might be an idea for another clean-up.
There was a problem hiding this comment.
Yes - that's an unfortunate choice of the existing interface.
I do plan to make a deeper change in 'main', and will probably try to change that.
The simplest solution - just strdup() that name. But I'll try to move to std::string-s if possible.
src/common/config_cacher.h
Outdated
| class md_config_cacher_t : public md_config_obs_t { | ||
| ConfigProxy& conf; | ||
| const char* keys[2]; | ||
| const char* const option_name; |
There was a problem hiding this comment.
Do we really need option_name after introducing keys?
There was a problem hiding this comment.
Good catch. Fixed.
ronen-fr
force-pushed
the
wip-rf-catcher-fix
branch
from
39ecc4b to
589e7de
Compare
|
@rzarzynski - can you take a look again? |
In its get_tracked_conf_keys() member function, the
cacher (in the existing code) initializes a static
function-block variable ('keys'), and uses it for
registering the observer.
But the cacher is instantiated on the type of
the configuration value. Thus, multiple cacher
objects for which the configuration values are
of the same type - share the static 'keys'. Only
one of the observers is registered.
Note that the code could have been simplified
somewhat, if the signature of the
get_tracked_conf_keys() function
was changed to return 'const char* const *'.
Fixes: https://tracker.ceph.com/issues/69236
Signed-off-by: Ronen Friedman <[email protected]>
Signed-off-by: Ronen Friedman <[email protected]>
moved out of the main commit to facilitate backporting it to pre-C++20. Signed-off-by: Ronen Friedman <[email protected]>
ronen-fr
force-pushed
the
wip-rf-catcher-fix
branch
from
589e7de to
5c09888
Compare
Signed-off-by: Ronen Friedman <[email protected]>
|
Merging based on my Teuthology tests. Tested under two branches (the updated one - rf-cacher-fix-4). |
|
jenkins test make check |
2 participants
In its get_tracked_conf_keys() member function, the cacher (in the existing code)
initializes a static function-block variable ('keys'), and uses it for
registering the observer.
But the cacher is instantiated on the type of
the configuration value. Thus, multiple cacher
objects for which the configuration values are
of the same type - share the static 'keys'. Only
one of the observers is registered.
Note that the code could have been simplified
somewhat, if the signature of the
get_tracked_conf_keys() function
was changed to return 'const char* const *'.
Fixes: https://tracker.ceph.com/issues/69236
Note:
Related to https://tracker.ceph.com/issues/69362
(which was errouneously marked as fixed by
#61184),
and its backport: https://tracker.ceph.com/issues/69374
(#61185)