Shift local_addr to not collade with the address #48
Conversation
After a recent refactoring, get_local_addr_port can produce the same address as the base server address. This of course doesn't work for clients, correct that.
erthalion
force-pushed
the
fix/shift-local-addr
branch
from
da1cccc to
575faa5
Compare
src/worker/network.rs
Outdated
| // The first resulting local address must not collide with the server | ||
| // addres, thus shift it by one right away. | ||
| if octets[3] != 255 { | ||
| octets[3] = octets[3].saturating_add(1); |
There was a problem hiding this comment.
I don't think this is the best way of doing this. You could increment addr before calling this function or you could add 1 to addr_index above or you could add 1 to octets[3] and set carry to 1 if octets[3] is 255.
I think saturating_add is not necessary if you have a check for octets[3] != 255. I don't think you should have that check or use saturating_add. You should still increment the IP address in that case.
There was a problem hiding this comment.
I agree that having a saturating_add right after a check doesn't make much sense.
On top of that, we might need to improve error handling here, because what happens if the octet is not modified? Will we get 2 colliding IP addresses? Does that work or will it cause something to break down the line?
w.r.t carrying the overflow over, that could cause the IP address to move to a different network, so that might also not be the desired behavior (although no handling of mask value is done here, so 🤷🏻 )
There was a problem hiding this comment.
saturating_add was suggested by clippy, so no idea why it was needed. After thinking more I think it would be even simpler if we do increment on bits (I've bumped smoltcp version for that).
Will we get 2 colliding IP addresses? Does that work or will it cause something to break down the line?
I don't think it would break anything, it's only a "fake" address in the end. The only reason why colliding with the server address can be a problem is that the testing setup may tread this ip differently (as it happens in prepare-tap.sh).
src/worker/network.rs
Outdated
| // The first resulting local address must not collide with the server | ||
| // addres, thus shift it by one right away. | ||
| if octets[3] != 255 { | ||
| octets[3] = octets[3].saturating_add(1); |
There was a problem hiding this comment.
I agree that having a saturating_add right after a check doesn't make much sense.
On top of that, we might need to improve error handling here, because what happens if the octet is not modified? Will we get 2 colliding IP addresses? Does that work or will it cause something to break down the line?
w.r.t carrying the overflow over, that could cause the IP address to move to a different network, so that might also not be the desired behavior (although no handling of mask value is done here, so 🤷🏻 )
There is no need for manual iteration over octets, do the increment in the bits form.
erthalion
force-pushed
the
fix/shift-local-addr
branch
from
52504d4 to
fe58584
Compare
3 participants
After a recent refactoring, get_local_addr_port can produce the same address as the base server address. This of course doesn't work for clients, correct that.