Closed Bug 451851 Opened 12 years ago Closed 12 years ago
Need a more reliable way to indicate SQLITE
right now it and SQLITE_LOCKED are mapped to NS_ERROR_FILE_IS_LOCKED, but that isn't a good error message for storage. I think it's high time we get some more useful status codes, starting with this one.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Whiteboard: [needs patch]
Whiteboard: [needs patch] → [has patch][needs review asuth]
Target Milestone: --- → mozilla1.9.1b2
Comment on attachment 345311 [details] [diff] [review] v1.0 Yeah, the semantics are rather different for the errors, so it's nice to have them mapped to different errors. Also, nice to not have two include files with the same name anymore. Since you're introducing a new error code for SQLITE_BUSY, I think it makes sense to modify mozStorageStatement::ExecuteStep to return it when it gets a SQLITE_BUSY. Right now it explicitly returns NS_ERROR_FAILURE (in the same case as SQLITE_MISUSE...) r=asuth with that. (nb: mxr for NS_ERROR_FILE_IS_LOCKED suggests no one currently cares about this error; not that it is likely to have been emitted with ExecuteStep converting it explicitly.)
Attachment #345311 - Flags: review?(bugmail) → review+
(In reply to comment #2) > Since you're introducing a new error code for SQLITE_BUSY, I think it makes > sense to modify mozStorageStatement::ExecuteStep to return it when it gets a > SQLITE_BUSY. Right now it explicitly returns NS_ERROR_FAILURE (in the same > case as SQLITE_MISUSE...) r=asuth with that. I would do that, except that consumers who are checking for exact error codes will break. As much as I want to be better about that, I think it might be better to wait for mozilla 2 for that kind of change (and making all the error codes map better in general).
Er, with that said, are you OK with landing as is?
Whiteboard: [has patch][needs review asuth] → [has patch][has review]
So, I buy your logic, but I'm not sure it's consistent. The return values of SQLITE_BUSY for mozStorageConnection::executeSimpleSQL and its users beginTransaction, commitTransaction, and rollbackTransaction change with this patch. Do you have a particular set of existing consumers that you know use ExecuteStep in this fashion but nothing else, or are we dealing with hypothetical cases? Having said that, I'm fine with landing this; an improvement is an improvement.
hmm, yeah. We should just update ExecuteStep. There is nothing in tree that I'm aware of.
I removed the |return NS_ERROR_FAILURE;| line when we check for SQLITE_BUSY or SQLITE_MISUSE, so it will fall out and end up calling ConvertResultCode.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
You need to log in before you can comment on or make changes to this bug.