cats: failed update message when updating all volumes from all pools#1015
Conversation
|
[suggestion] inject a small test ? Like create an empty pool, make a default backup to create a volume in default. change retention time parameter in default then run the command and check retention time changed for previous volumes + no error |
8c97898 to
ce4a75f
Compare
ce4a75f to
6fdbb8c
Compare
arogge
left a comment
There was a problem hiding this comment.
There's a problem with the CHANGELOG.md and there's potential for some refactoring in UPDATE_DB() that you might consider.
core/src/cats/cats.h
Outdated
| #define UPDATE_DB_NO_AFFECTED_ROWS(jcr, cmd) \ | ||
| UpdateDB(__FILE__, __LINE__, jcr, cmd, 0) |
There was a problem hiding this comment.
While this is a lot better than before, the naming is still bad.
Just from the naming I'd guess that UPDATE_DB() runs an update query on the database without any further checks (besides making sure the query did not fail).
For UPDATE_DB_NO_AFFECTED_ROWS() I'd guess it does the same as UPDATE_DB() but won't return the number of affected rows.
I think it would be a lot easier to understand if we changed UPDATE_DB() and BareosDB::UpdateDB() to
- not to take the additional parameter for minimum affected rows
- return the number of affected rows as an
intand-1on error.
This way the existing calls to UPDATE_DB() that look like if(UPDATE_DB(...)) would have to be changed to if(UPDATE_DB(...) > 0) (i.e. at least one row was affected) and UPDATE_DB_NO_AFFECTED_ROWS() could be done with either >= 0 or != -1.
Same is probably true for INSERT_DB(), which currently fails if you do a multi-row insert like INSERT INTO x (col_name) VALUES (row1_value), (row2_value), (row3_value); or INSERT INTO y SELECT * FROM z;. So for keeping the existing behaviour you'd need if(INSERT_DB(...) == 1).
1086a0d to
5fbf69c
Compare
arogge
left a comment
There was a problem hiding this comment.
There are some unneccessary constructs, that should probably be removed.
Other than that I'm not sure how far you want to go with this, but I have another refactoring, that might be worth a shot:
Currently we have a lot of DbLock(this) and DbUnlock(this). Especially in constructs like
DbLock(this);
int retval = SOMETHING_DB(...);
DbUnlock(this);
return retval;
This could be simplified if we had an RAII-Container for DbLock()/DbUnlock() like this:
class DbLocker {
BareosDb* db_handle_;
public:
DbLocker(BareosDb* db_handle) : db_handle_(db_handle) {
DbLock(db_handle_)
}
~DbLocker() {
DbUnlock(db_handle_);
}
}
because then you could refactor the above code into:
DbLocker _{this};
return SOMETHING_DB(...);
As DbLocker will handle unlocking when it goes out-of-scope, we can return without the intermediate variable.
5fbf69c to
3dee692
Compare
3dee692 to
22d6c7f
Compare
arogge
left a comment
There was a problem hiding this comment.
Incredible work!
I admit that the simple change escalated quite a lot, but I think removing 86 gotos and saving around 300 lines of code was probably worth it.
Description:
When trying to update all volumes from all pools using the update command, while not having any volumes configured, an error message is displayed mentioning that no rows were affected. The behavior is normal, but the message is misplaced. This PR fixes the issue.
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)
General
Source code quality
bareos-check-sources --since-mergedoes not report any problemsgit statusshould not report modifications in the source tree after building and testingTests