retry arping one more time in ifup-eth when sendto failed #490
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Honestly, I don't like the "try things again if they fail" approach. It is not a sign of good design. But the network-scripts always had ton of hacks, so I am probably fine with adding one. But please try to do this a bit nicer; copying a code and putting it in else close under is ugly as hell. |
|
You're right, its ugly. So i refactor it with ARPING_TRIES. Please review this PR again. |
network-scripts/ifup-eth
Outdated
| net_log $"Error, some other host ($ARPINGMAC) already uses address ${ipaddr[$idx]}." | ||
| exit 1 | ||
| fi | ||
| while [ $tries -le $ARPING_TRIES ] |
There was a problem hiding this comment.
We tend to keep "while ...; do" on one line. Also, it is a good practice to always put variables in the statement into double quotes
network-scripts/ifup-eth
Outdated
| ARPINGMAC=$(echo $ARPING | sed -ne 's/.*\[\(.*\)\].*/\1/p') | ||
| net_log $"Error, some other host ($ARPINGMAC) already uses address ${ipaddr[$idx]}." | ||
| tries=$((tries+1)) | ||
| if [ $tries -gt $ARPING_TRIES ] |
There was a problem hiding this comment.
Same as above, this should be checked after the cycle. And again missing double quotes
network-scripts/ifup-eth
Outdated
| tries=$((tries+1)) | ||
| if [ $tries -gt $ARPING_TRIES ] | ||
| then | ||
| net_log "arping failed after $tries tries" |
There was a problem hiding this comment.
Missing $ so the string could be translated ($"string")
|
And please squash your commits. |
1 similar comment
I don't think this was addressed. |
We now only print "Error, some other host ".... once there really is an $ARPINGMAC in output of ARPING. In this case we exit the loop directly so there would not be a repeated Error msg print. |
Ah, I missed the break, sorry. |
network-scripts/ifup-eth
Outdated
| break | ||
| fi | ||
| tries=$((tries+1)) | ||
| else |
There was a problem hiding this comment.
Can you flip this a bit? I think this would be nicer:
ARPING=$(/sbin/arping -c 2 -w ${ARPING_WAIT:-3} -D -I ${REALDEVICE} ${ipaddr[$idx]})
[ $? = 0 ] && break
ARPINGMAC=$(echo $ARPING | sed -ne 's/.[(.)].*/\1/p')
....
network-scripts/ifup-eth
Outdated
| net_log $"Error, some other host ($ARPINGMAC) already uses address ${ipaddr[$idx]}." | ||
| if [ -n "${ARPINGMAC}" ]; then | ||
| net_log $"Error, some other host ($ARPINGMAC) already uses address ${ipaddr[$idx]}." | ||
| break |
There was a problem hiding this comment.
Shouldn't it be here "exit 1" instead of "break"? If we find a duplicate address on the first iteration loop, then the if [ "${tries}" -gt "${ARPING_TRIES}" ]; check below will be false, and we will continue with the rest of the code.
|
@lnykryn All done, plz check. |
|
I still think that there should be exit 1 instead of break on line 302 in your code |
YES, its wrong to use break, fixed. |
retry arping one more time in ifup-eth when sendto failed