Remove DB_ERROR_WRONG_NUM_FIELDS and ensure TXT_DB_read always prints a message for an error #17060
Conversation
This never worked since it was introduced in 51b04a6 because TXT_DB_read always returns NULL when there's an error, so the 'ret' struct containing the DB_ERROR_WRONG_NUM_FIELDS error gets discarded. All other functions in this file are passed in a TXT_DB * and can indicate the existance an error in their return value. TXT_DB_read acts differently because it returns a TXT_DB * or a NULL in the case of an error. CLA: trivial
All failures in load_index will now print out an error message. CLA: trivial
paulidale
added
approval: otc review pending
paulidale
added
approval: review pending
|
IMO this is really borderline - especially adding the message part. Would you please submit a regular CLA? |
| @@ -27,7 +27,6 @@ | |||
| # define DB_ERROR_INDEX_OUT_OF_RANGE 3 | |||
| # define DB_ERROR_NO_INDEX 4 | |||
| # define DB_ERROR_INSERT_INDEX_CLASH 5 | |||
| # define DB_ERROR_WRONG_NUM_FIELDS 6 | |||
There was a problem hiding this comment.
This is an API break and so it would not be acceptable earlier than in 4.0.
Why do you remove this particular error?
There was a problem hiding this comment.
Ah, it isn't really returned anywhere. But then OK, just keep the define here and perhaps add a comment, that it is never really returned.
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
paulidale
added
hold: cla required
|
This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
|
This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
|
This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 90 days. |
This makes two changes in related code for loading index.txt:
DB_ERROR_WRONG_NUM_FIELDSdefine which was never actually returned fromTXT_DB_read(see commit message for details).load_indexprint out an error message. The changes introduced in apps/openssl: make index.txt errors more verbose #15360 already ensure that all callers ofload_indexwill also print out a message, this just adds another level of detail inside of load_index. Note: I'm not familiar with ERR_raise_data, so I did not follow that pattern that was used elsewhere in this function. Please feel free to further tweak or I'm happy to take suggestions to improve this.An alternative to this PR is the work I first did in this branch: https://github.com/openssl/openssl/compare/master...deejgregor:fix-txt-db-read-failure-reporting?expand=1 . That, however, feels too verbose for the value added and it seems like a refactoring to use ERR_raise_data would be better to get that level of specificity of errors, and I'm not sure it is worth it for this code (and my lack of familiarity with ERR_raise_data would also make this more complex for me to complete). So, I instead propose the simpler solution in this PR.
Checklist