Skip to content

Add error log message when failing to open RDB file for reading#11036

Merged
oranagra merged 1 commit intoredis:unstablefrom
YaacovHazan:log-fail-rdb-open
Jul 25, 2022
Merged

Add error log message when failing to open RDB file for reading#11036
oranagra merged 1 commit intoredis:unstablefrom
YaacovHazan:log-fail-rdb-open

Conversation

@YaacovHazan
Copy link
Collaborator

@YaacovHazan YaacovHazan commented Jul 25, 2022 •
edited by oranagra
Loading

When failing to open the rdb file, there was no specific error printed (unlike a corrupt file), so it was not clear what failed and why.

@oranagra oranagra merged commit 33bd8fb into redis:unstable Jul 25, 2022
@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Jul 26, 2022 •
edited
Loading

look like it somehow break the alpine test:

4312:M 26 Jul 2022 00:16:57.254 # Fatal error: can't open the RDB file dump.rdb for reading: No such file or directory
4312:M 26 Jul 2022 00:16:57.254 # Fatal error loading the DB: Invalid argument. Exiting.

https://github.com/redis/redis/runs/7511034530?check_suite_focus=true#step:7:93
https://github.com/redis/redis/runs/7511034613?check_suite_focus=true#step:7:93

odd, it look errno will change, i tested it like this, doing a int save_errno = errno can make it work (or we ignore )

        serverLog(LL_WARNING,"1 Fatal error: can't open the RDB file %s for reading: %s", filename, strerror(errno));
        serverLog(LL_WARNING,"2 Fatal error: can't open the RDB file %s for reading: %s", filename, strerror(errno));
        serverLog(LL_WARNING,"3 Fatal error: can't open the RDB file %s for reading: %s", filename, strerror(errno));

2427:M 26 Jul 2022 03:06:59.382 # 1 Fatal error: can't open the RDB file dump.rdb for reading: No such file or directory
2427:M 26 Jul 2022 03:06:59.382 # 2 Fatal error: can't open the RDB file dump.rdb for reading: Invalid argument
2427:M 26 Jul 2022 03:06:59.382 # 3 Fatal error: can't open the RDB file dump.rdb for reading: Invalid argument
2427:M 26 Jul 2022 03:06:59.382 # Fatal error loading the DB: Invalid argument. Exiting.

@oranagra
Copy link
Member

oranagra commented Jul 26, 2022

ohh, i missed the fact the caller checks the errno already, and now the logging corrupts errno.
this means that we should either revert the new print (arguably already covered), or we need to backup and restore errno.
however, I don't think it's safe to rely on errno as we do today, because other cases in the code (who run into a read error or corrupt rdb) also do log printing, or call other system calls that can return ENOENT for other cases (not a missing rdb file).

so i think we should either:

  1. make sure that rdbLoad always sets errno before returning, and avoids setting ENOENT on anything other than the fopen.
  2. change the return value of that function to an enum, like we have when loading an AOF.

@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Jul 26, 2022 •
edited
Loading

i prefer option 2 (like #9012), or if we want a simple fix, we can just if (errno == ENOENT) return C_ERR; in this case

i try to make it (option 2) happen in #11039

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 26, 2022
@enjoy-binbin
Change the return value of rdbLoad function to enums
0c6b698 
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 mentioned this pull request Jul 26, 2022
Merged
oranagra pushed a commit that referenced this pull request Jul 26, 2022
@enjoy-binbin
Change the return value of rdbLoad function to enums (#11039)
00097bf 
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.
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@YaacovHazan @enjoy-binbin
Add error log message when failing to open RDB file for reading (redi…
9922c57 
…s#11036)

When failing to open the rdb file, there was no specific error printed (unlike a
corrupt file), so it was not clear what failed and why.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@enjoy-binbin
Change the return value of rdbLoad function to enums (redis#11039)
8f5e915 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@YaacovHazan @enjoy-binbin @oranagra