cats: add missing database locks#1787
Merged
BareosBot merged 24 commits intobareos:masterfrom Aug 20, 2024
Merged
Conversation
aabaf91 to
c83d9a4
Compare
c83d9a4 to
f438920
Compare
arogge
requested changes
Jun 7, 2024
Member
arogge
left a comment
There was a problem hiding this comment.
I'm not happy with the amount of calls to AssertOwnership(), especially as it looks pretty expensive. However, I don't have a great idea how to improve it right now.
I'll see if I can come up with an idea over the weekend...
92b35fc to
501bfa6
Compare
Contributor
Author
|
I had to rebase onto master to make it build again. Sorry! |
501bfa6 to
122edcb
Compare
7bbc092 to
385b2dc
Compare
arogge
approved these changes
Aug 19, 2024
arogge
approved these changes
Aug 19, 2024
Member
arogge
left a comment
There was a problem hiding this comment.
Besides the year in cats.cc's header, I think this is fine now.
| Copyright (C) 2011-2011 Free Software Foundation Europe e.V. | ||
| Copyright (C) 2011-2016 Planets Communications B.V. | ||
| Copyright (C) 2013-2023 Bareos GmbH & Co. KG | ||
| Copyright (C) 2013-2024 Bareos GmbH & Co. KG |
Member
There was a problem hiding this comment.
If we don't have any change, this should also go away (after merging the fixups).
Contributor
Author
There was a problem hiding this comment.
Merged the fixups now. Lets see what jenkins has to say.
385b2dc to
b926bc8
Compare
This function is not safe to use if you dont actually own the lock, but misuse should hopefully lead to a crash anyways.
This is not checked in case of private connections since they are not shared automatically. It probably makes sense to also check them since they might get shared by accident (e.g. setting a pointer in jcr and then another thread accessing that jcr).
As this is always a pointer into private PQresult data, we should make sure (at compile time) that we do not write into it.
This also removes the need to AssertOwnership() inside of it
These functions are just used internally, so we make them private.
As this is a private function, it can assume that everything is ok.
Numeric values should also be printed just like other numbers.
db->strerror() should only be called with the lock held unless the db
is private. 90% of the time db calls look like
```
if (!db->Function()) {
Error(db->strerror());
}
```
In these cases we can need to lock the db during the if, otherwise we
could read stale info (or even run into a race condition as strerror
is unsynchronized).
49e4899 to
a3e0c1d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for contributing to the Bareos Project!
By default, bareos db connections get shared by multiple jobs/threads. It is of utmost importance to ensure that those shared connections only get written to/read from if the calling thread has acquired the lock.
This PR by no means ensures that this is done correctly everywhere, but the start of an ongoing process to weed out every race condition in the database code.
This PR adds some missing locks as well as assertions that locks were taken correctly at the relevant places.
Additionally it renames
bac_cursor_tobar_cursor_.Some unused database code was also removed as well as making the
numerictype count as a numeric type.Please check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
Make sure you check/merge the PR using
devtools/pr-toolto have some simple automated checks run and a proper changelog record added.General
Required backport PRs have been createdSource code quality