Skip to content

Change the return value of rdbLoad function to enums#11039

Merged
oranagra merged 3 commits intoredis:unstablefrom
enjoy-binbin:rdb_load_ret
Jul 26, 2022
Merged

Change the return value of rdbLoad function to enums#11039
oranagra merged 3 commits intoredis:unstablefrom
enjoy-binbin:rdb_load_ret

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Jul 26, 2022

The reason we do this is because in #11036, we added error
log message when failing to open RDB file for reading.
In loadDdataFromDisk we call rdbLoad and also check errno,
now the logging corrupts errno (reported in alpine daily).

It is not safe to rely on errno as we do today, so we change
the return value of rdbLoad function to enums, like we have
when loading an AOF.

The reason we do this is because in redis#11036, we added error
log message when failing to open RDB file for reading.
In loadDdataFromDisk we call rdbLoad and also check errno,
now the logging corrupts errno (reported in alpine daily).

It is not safe to rely on errno as we do today, so we change
the return value of rdbLoad function to enums, like we have
when loading an AOF.
@enjoy-binbin enjoy-binbin marked this pull request as ready for review July 26, 2022 11:27
@oranagra oranagra merged commit 00097bf into redis:unstable Jul 26, 2022
@enjoy-binbin enjoy-binbin deleted the rdb_load_ret branch July 26, 2022 14:24
@oranagra
Copy link
Member

@enjoy-binbin, Yaacov noticed that we forgot to remove the errno print from the outer message (may be invalid since errno may have been overwritten).

i suppose our aim should be the the code that detects the error and knows which system call triggered it, is the one to print errno, and not the code way up above (in some cases a result of a logical error and not a system one).
so let's trim that from the print.

additionally, the doc comment of rdbLoadRioWithLoadingCtx mentions that it updates the errno, but i don't think it's true for all cases. and i'm not sure anyone relies on it (to execute specific logic), so maybe we can just trim this comment too?
and the print that prints it in one of it's callers...

@enjoy-binbin
Copy link
Contributor Author

we forgot to remove the errno print from the outer message (may be invalid since errno may have been overwritten).

yep.. i did check the RDB_NOT_EXIST one but forget the RDB_FAILED
I took a look at the errno around it, and made a PR: #11042, i will take a deep look late and update the top comment

oranagra pushed a commit that referenced this pull request Aug 4, 2022
This is an addition to #11039, which cleans up rdbLoad* related errno. Remove the
errno print from the outer message (may be invalid since errno may have been overwritten).

Our aim should be the code that detects the error and knows which system call
triggered it, is the one to print errno, and not the code way up above (in some cases
a result of a logical error and not a system one).

Remove the code to update errno in rdbLoadRioWithLoadingCtx, signature check
and the rdb version check, in these cases, we do print the error message.
The caller dose not have the specific logic for handling EINVAL.

Small fix around rdb-preamble AOF: A truncated RDB is considered a failure,
not handled the same as a truncated AOF file.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
The reason we do this is because in redis#11036, we added error
log message when failing to open RDB file for reading.
In loadDdataFromDisk we call rdbLoad and also check errno,
now the logging corrupts errno (reported in alpine daily).

It is not safe to rely on errno as we do today, so we change
the return value of rdbLoad function to enums, like we have
when loading an AOF.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
This is an addition to redis#11039, which cleans up rdbLoad* related errno. Remove the
errno print from the outer message (may be invalid since errno may have been overwritten).

Our aim should be the code that detects the error and knows which system call
triggered it, is the one to print errno, and not the code way up above (in some cases
a result of a logical error and not a system one).

Remove the code to update errno in rdbLoadRioWithLoadingCtx, signature check
and the rdb version check, in these cases, we do print the error message.
The caller dose not have the specific logic for handling EINVAL.

Small fix around rdb-preamble AOF: A truncated RDB is considered a failure,
not handled the same as a truncated AOF file.
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