no one ever said that the config file supports this, but you're right that someone may try to do that by mistake.
i think the right solution is to check that that atoi failed, and goto loaderr (redis should fail to start).
what do you think?

OK,as long as it can avoid redis trying to connect to no:0.😃

So do you wanna do that (change the PR)?

Do you mean submitting another PR or directly changing the current PR？(sorry,i'm not experienced in submitting pull requests😞)

You can push another commit into the same branch, or amend the commit and push - f

Looks like CONFIG REWRITE is generating such a config, and changing that is wrong since there are already many config like that out there.
https://github.com/redis/redis/runs/1164542228?check_suite_focus=true
Looks like we're gonna have to revert to your previous fix.
And also check that port of 0 means the same (nullify masterhost or skip the line)

So,we're gonna revert to the previous fix?

How about checking if the port is either "one" (I.e "no one") or "0" (I.e. "127.0.0.1 0") , skip the config like your original fix, but anything else that failed the atoi causing an error?

I think if the port is "0",we'll set it to server.masterport,otherwise,anything else that failed the atoi causing an error,right?

I think the current (old) behaviour which sets masterhost to non-NULL when getting "127.0.0.1 0" is wrong.
So I think "no one" and "127.0.0.1 0" should pass (leaving server.masterhost unchanged (or setting it to Null), and anything else should error.

Come to think of it, explicitly setting to NULL is better, since it might override a previous statement in the config file, so override is better than skipping.

So,we are going to skip "slaveof no one" and "slaveof 127.0.0.1 0" and set server.masterport to NULL,otherwise,anything else that failed the atoi causing an error.right? I think this is a thorough consideration.

Sounds like a good plan.
Setting masterhost to NULL (not masterport)
and masterport to 0

Just setting masterhost to NULL is enough,i think.

It seems that we can't set masterhost to NULL when getting "slaveof 127.0.0.1 0",some tests would fail.

i'm sorry, i didn't dig deep enough to see the reason for the previous failure and assumed CONFIG REWRITE generates "127.0.0.1 0" when there's no master.
but i now realized it's a totally different issue. there's a test doing that on purpose in order to create a replica that's unable to connect to it's master.
both failures were actually in the same test, once failing to restart and the other time failing because it became a master:

        # Prevent the slave from receiving master updates, and at
        # the same time send a new script several times to the
        # master, so that we'll end with EVALSHA into the backlog.
        $R($slave_id) slaveof 127.0.0.1 0

        $R($master_id) EVALSHA e6e0b547500efcec21eddb619ac3724081afee89 0
        $R($master_id) EVALSHA e6e0b547500efcec21eddb619ac3724081afee89 0
        $R($master_id) EVALSHA e6e0b547500efcec21eddb619ac3724081afee89 0

        catch {
            $R($slave_id) config rewrite
            restart_server [expr {0-$slave_id}] true
            set R($slave_id) [srv [expr {0-$slave_id}] client]
        }

so since 0 is a now a valid port number, i would like to suggest the following

1. if the input is no one nullify masterhost and continue
2. use strtol instead of atoi so that we can detect when the input is not a number.
3. if strtol failed, goto loaderr

I looked at the code for setting the port and found that the author did not verify whether the port is a valid number, but only verified whether the port is between 0 and 65535. Maybe we can maintain a consistent style with it. What do you think?

@CaoZB what do you mean by that?

I mean
1.if the input is no one nullify masterhost and continue
2.we don't check whether masterport in redis.conf is a valid number
3.just check whether server.msterport is between 0 and 65535 after parsing

isn't that exactly what i suggested in my 1,2,3 above?
i.e. in 3 we want to catch both numbers outside the range 0..64k, but also be able to fail when the input s not a number at all (atoi doesn't let us do that)

@oranagra Updated,please review it.

LGTM😄

@CaoZB sorry for the ping-pong, please have another looks at my last two commits.

😄I didn't even notice the memory leak.👍

Thank you for your precious time very much.😄