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

fcmp: Fix bogus "query failed" message #2208

Merged
merged 6 commits into from
Mar 16, 2024

Conversation

blabber
Copy link
Collaborator

@blabber blabber commented Feb 16, 2024

When the modpack installer updates an already installed modpack, it emits a bogus critical message that a query failed with error code 100.

Error code 100 is SQLITE_ROW and is not really an error, but signals that a data row has been successfully read. SQLITE_ROW is also an expected return value in mpdb_update_modpack.

Change the check for failed queries to ignore SQLITE_ROW.

When the modpack installer updates an already installed modpack, it
emits a bogus critical message that a query failed with error code 100.

Error code 100 is `SQLITE_ROW` and is not really an error, but signals
that a data row has been successfully read. `SQLITE_ROW` is also an
expected return value in `mpdb_update_modpack`.

Change the check for failed to queries to ignore `SQLITE_ROW`.
@blabber blabber force-pushed the bugifx/bogus_query_failed branch from 8a1df08 to 5bf53ad Compare February 16, 2024 22:41
Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

From the docs, sqlite3_step seems to return SQLITE_ROW in some cases, which is also not handled. It also seems we always need to call sqlite3_finalize (even when the query failed), or we have a memory leak.

@blabber
Copy link
Collaborator Author

blabber commented Feb 16, 2024

That's what I get for trying to get rid of a misleading message :D
I'll take a closer look at the sqlite handling next week.

@blabber blabber marked this pull request as draft February 16, 2024 23:02
@lmoureaux
Copy link
Contributor

I didn't require changes at this point, just want to understand better 😂 We can also open an issue to check

Make sure that every prepared statement in `mpdb_query` is finalized.
This changes the return value as an side effect (see below), change
return value checks accordingly.

This commit changes the return value of `mpdb_query`, as the old code
contained this special handling for `ret == SQLITE_DONE`:

```
if (ret == SQLITE_DONE) {
  ret = sqlite3_finalize(stmt);
}
```

This not only finalized the prepared statement, but also changed `ret`
to `SQLITE_OK`. After this commit `mpdb_query` returns either
`SQLITE_DONE` or `SQLITE_ROW`, instead of `SQLITE_OK` or `SQLITE_ROW`.
Make sure that every prepared statement in `mpdb_installed_version` is
finalized. After finalizing the pointer returned by
`sqlite3_column_text` becomes invalid and we must duplicate the string.
No functional changes.
Delete the strings returned by `mpdb_installed_version`.
@lmoureaux
Copy link
Contributor

Thanks for the improved memory handling! Am I officially allowed to run a build with -fsanitize=leak ? 👹

@blabber
Copy link
Collaborator Author

blabber commented Feb 22, 2024

Feel free :) I don't think I've made anything worse, but I don't know what surprises might linger in the code.

And I'm not sure whether the QStrings will be deleted at some point. I don't really know QT, but I read something about reference counting.

@lmoureaux
Copy link
Contributor

I'm not sure whether the QStrings will be deleted at some point. I don't really know QT, but I read something about reference counting.

A QString created on the stack (so without a pointer) will automatically free its resources when going out of scope at the end of the function. There is some internal magic to make copying more efficient, but we don't need to care about this.

@blabber blabber marked this pull request as ready for review February 22, 2024 21:06
Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

ASan didn't detect any build (not that it detected any before - I suppose closing the sqlite session frees the resources as well)

@lmoureaux lmoureaux merged commit 339abcd into longturn:master Mar 16, 2024
20 of 21 checks passed
@blabber blabber deleted the bugifx/bogus_query_failed branch March 16, 2024 23:06
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.

2 participants