| Summary: | sqlite: show extended error codes in error logs | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Evan Martin <evan> | ||||||
| Component: | New Bugs | Assignee: | Evan Martin <evan> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, ddkilzer, dumi, jorlow, tony | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Evan Martin
2010-10-18 16:09:54 PDT
Created attachment 71098 [details]
Patch
This helped me track down http://code.google.com/p/chromium/issues/detail?id=59420 Comment on attachment 71098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review > WebCore/platform/sql/SQLiteDatabase.cpp:76 > + m_lastError = sqlite3_extended_result_codes(m_db, 1); I'm not sure why m_lastError is a member variable. It doesn't appear to be used other than in the open function. Also, it looks like we only get here when m_lastError == SQLITE_OK. Will sqlite3_extended_result_codes provide information when SQLITE_OK? It seems weird that sqlite3_extended_result_codes returns the previous error. Comment on attachment 71098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review r=me, but please address the questions below before committing. >> WebCore/platform/sql/SQLiteDatabase.cpp:76 >> + m_lastError = sqlite3_extended_result_codes(m_db, 1); > > I'm not sure why m_lastError is a member variable. It doesn't appear to be used other than in the open function. > > Also, it looks like we only get here when m_lastError == SQLITE_OK. Will sqlite3_extended_result_codes provide information when SQLITE_OK? It seems weird that sqlite3_extended_result_codes returns the previous error. Making m_lastError a local variable should be a separate patch, but I agree that it doesn't need to be a member variable. The sqlite3_extended_result_codes() function enables extended error codes for future database operations. My only concern is whether enabling extended error codes has any performance impact. (I suspect it doesn't because the error codes are just integers.) > WebCore/platform/sql/SQLiteStatement.cpp:108 > + error, m_query.ascii().data(), error, sqlite3_errmsg(m_database.sqlite3Handle())); Isn't printing 'error' twice here redundant? Created attachment 71313 [details]
Patch
Comment on attachment 71313 [details]
Patch
Carrying over ddkilzer's previous r+.
Comment on attachment 71313 [details] Patch Clearing flags on attachment: 71313 Committed r70219: <http://trac.webkit.org/changeset/70219> All reviewed patches have been landed. Closing bug. Comment on attachment 71313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71313&action=review > WebCore/platform/sql/SQLiteDatabase.cpp:80 > + if (sqlite3_extended_result_codes(m_db, 1) != SQLITE_OK) { > + LOG_ERROR("SQLite database error when enabling extended errors - %s", sqlite3_errmsg(m_db)); > + sqlite3_close(m_db); > + m_db = 0; > + return false; > + } I'm not sure if a failure to enable extended result codes should cause the database open to fail, but I suppose it will be interesting to know when it doesn't work. No change is required--I just noticed this after Tony re-added my r+ to this patch. Heh, just looked at the sqlite source.
int sqlite3_extended_result_codes(sqlite3 *db, int onoff){
sqlite3_mutex_enter(db->mutex);
db->errMask = onoff ? 0xffffffff : 0xff;
sqlite3_mutex_leave(db->mutex);
return SQLITE_OK;
}
So I guess that is dead code. :\
I imagine they return the error code to leave it open to potentially returning errors in the future. I wonder what sort of errors they could be.
|