Skip to content
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

Don't load folder if we encounter a db error #9477

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Conversation

TheOneRing
Copy link
Member

Fixes: #9147

@TheOneRing TheOneRing requested a review from a team March 1, 2022 13:11
@sonarcloud
Copy link

sonarcloud bot commented Mar 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing added this to the 2.10.1 milestone Mar 1, 2022
_error = QString::fromUtf8(sqlite3_errmsg(_db)); \
} \
namespace {
constexpr auto SQLITE_SLEEP_TIME = 500ms;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sleep is probably not needed as it seems that the sqlite calls them self do block for a certain amount of time.

Copy link
Contributor

@fmoc fmoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a relatively large change. I can't really tell whether this fixes the mentioned issue from the code, but it does seem to fix a few corner cases.

src/common/ownsql.cpp Show resolved Hide resolved
}
return false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the main issue, if we failed to prepare the statement we just continued.

close();
QFile::remove(filename);
QFile fileToRemove(filename);
if (!fileToRemove.remove()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If removing the broken db failed we are not ready...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Do we have to implement additional error handling?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No just return false, the code was meant to fix a corrupted db by removing it.
However if removing the broken db fails.... this pr provides the required error handling.

close();
QFile::remove(filename);
QFile fileToRemove(filename);
if (!fileToRemove.remove()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Do we have to implement additional error handling?

@TheOneRing TheOneRing merged commit d47a148 into 2.10 Mar 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/handle_locked_db branch March 10, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants