retry arping one more time in ifup-eth when sendto failed

[xanoxes]

retry arping one more time in ifup-eth when sendto failed

[lnykryn]

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.
What I could imagine is a new ARPING_RETRY or ARPING_TRIES option and then looping over arping until the limit is hit. And this would also allow not to change the behaviour and keep this opt-in.

[xanoxes]

You're right, its ugly. So i refactor it with ARPING_TRIES. Please review this PR again.

[lnykryn]

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

[lnykryn]

Same as above, this should be checked after the cycle. And again missing double quotes

[lnykryn]

Missing $ so the string could be translated ($"string")

[lnykryn]

And please squash your commits.

[xanoxes]

@lnykryn

[xanoxes]

@lnykryn

[lnykryn]

This will cause that "Error, some other host ".... message to be needlessly printed several times. It would be better to structure the code something like this: while .. do arping ret=$? done if [ ret =1 ] ....

I don't think this was addressed.

[xanoxes]

This will cause that "Error, some other host ".... message to be needlessly printed several times. It would be better to structure the code something like this: while .. do arping ret=$? done if [ ret =1 ] ....

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.

[lnykryn]

This will cause that "Error, some other host ".... message to be needlessly printed several times. It would be better to structure the code something like this: while .. do arping ret=$? done if [ ret =1 ] ....

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.

[lnykryn]

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')
....

[lnykryn]

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.

[xanoxes]

@lnykryn All done, plz check.

[lnykryn]

I still think that there should be exit 1 instead of break on line 302 in your code

[xanoxes]

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.

[xanoxes]

@lnykryn ALL fixed, plz check

[jamacku]

I would rather see the use of a for loop instead of a while loop. Also, you are missing the initialization of tries and ARPING_TRIES

Suggested change

	while [ "${tries}" -le "${ARPING_TRIES}" ]; do
	for (( tries=0; tries<${ARPING_TRIES:=1}; tries++ )); do

[jamacku]

It will be handled in the head of the for loop.

Suggested change

tries=$((tries+1))

[jamacku]

When using for loop, you need to update this condition

Suggested change

	if [ "${tries}" -gt "${ARPING_TRIES}" ]; then
	if [ "${tries}" -eq "${ARPING_TRIES}" ]; then

[jamacku]

Suggested change

	[ $? = 0 ] && break
	[ "$?" -eq 0 ] && break