596 | +static fs::path g_blocks_path_cache_net_specific GUARDED_BY(g_path_cache_mutex); 597 | +static fs::path pathCached GUARDED_BY(g_path_cache_mutex); 598 | +static fs::path pathCachedNetSpecific GUARDED_BY(g_path_cache_mutex); 599 | 600 | -const fs::path &GetBlocksDir() 601 | +namespace {
Why is this namespace required?
ACK f7f41a08
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Reworked without introducing a new helper function.
ping @vasild @promag @MarcoFalke
695 |
696 | const fs::path &GetBlocksDir()
697 | {
698 | - LOCK(csPathCached);
699 | + LOCK(g_blocksdir_path_mutex);
700 | fs::path &path = g_blocks_path_cache_net_specific;
Assuming g_blocksdir_path_mutex protects just the variable g_blocks_path_cache_net_specific, would it be possible to release the mutex after this assignment?
If "yes", then I would suggest to do it as a good practice of minimizing the time a mutex is held and also to avoid holding it together with the other newly introduced g_datadir_path_mutex.
If "no" then I think it is warranted to explain in a comment next to g_blocksdir_path_mutex what exactly is protected by it.
Assuming
g_blocksdir_path_mutexprotects just the variableg_blocks_path_cache_net_specific, would it be possible to release the mutex after this assignment?
I guess not, as the path references to the same memory location as the g_blocks_path_cache_net_specific variable, that must be protected by the g_blocksdir_path_mutex.
If "no" then I think it is warranted to explain in a comment next to
g_blocksdir_path_mutexwhat exactly is protected by it.
I think GUARDED_BY is pretty self-documented annotation, no?
oh well, I overlooked the fact that it is being assigned to a reference
Looks good.
The commit message reads just
util: Get rid of RecursiveMutex in Get{Blocks,Data}Dir
maybe it would be good to explain that we remove the recursive mutex by splitting it to two mutexes and ensuring they are always acquired in the same order.
This change removes the RecursiveMutex object by splitting it into two
Mutex objects, and ensuring they are always acquired in the same order.
-BEGIN VERIFY SCRIPT-
sed -i 's/pathCachedNetSpecific/g_datadir_path_cached_net_specific/' src/util/system.cpp
sed -i 's/pathCached/g_datadir_path_cached/' src/util/system.cpp
-END VERIFY SCRIPT-
Updated 0173e6884568bf9f03d08b80ffc0b80729378e44 -> 7b90f77146d36ef21760f2ce3704e2f2f463b3a2 (pr19213.02 -> pr19213.03, diff):
Looks good.
The commit message reads just
util: Get rid of RecursiveMutex in Get{Blocks,Data}Dir
maybe it would be good to explain that we remove the recursive mutex by splitting it to two mutexes and ensuring they are always acquired in the same order.
ACK 7b90f7714
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Why close?