Skip to content

errno cleanup around rdbLoad#11042

Merged
oranagra merged 3 commits intoredis:unstablefrom
enjoy-binbin:cleanup_rdbLoad_errno
Aug 4, 2022
Merged

errno cleanup around rdbLoad#11042
oranagra merged 3 commits intoredis:unstablefrom
enjoy-binbin:cleanup_rdbLoad_errno

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Jul 26, 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
Copy link
Contributor Author

i was thinking do we need the print in RDB_NOT_EXIST case?

    fp = fopen(filename, "r");
    if (fp == NULL) {
        retval = (errno == ENOENT) ? RDB_NOT_EXIST : RDB_FAILED;
        serverLog(LL_WARNING,"Fatal error: can't open the RDB file %s for reading: %s", filename, strerror(errno));
        return retval;
    }

like it will sometimes produce this, somehow like #10757

682:M 27 Jul 2022 10:41:49.202 # Fatal error: can't open the RDB file dump.rdb for reading: No such file or directory

@oranagra
Copy link
Member

@enjoy-binbin you are absolutely right.
@YaacovHazan and I were eager to "fix" the problem of this function returning an error without printing anything, that we missed the fact it's not an error.
now that we have a proper return enum, i hope it's clear and that mistake won't happen again.
let's drop that print!

@oranagra oranagra merged commit 4505eb1 into redis:unstable Aug 4, 2022
@enjoy-binbin enjoy-binbin deleted the cleanup_rdbLoad_errno branch August 4, 2022 07:48
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

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants