search

mpalmer / lvmsync

Synchronise LVM LVs across a network by sending only snapshotted changes
http://theshed.hezmatt.org/lvmsync
GNU General Public License v3.0
382 stars 61 forks source link

htonq appears to be implemented wrong #6

Closed nats closed 10 years ago

nats commented 11 years ago

Network order is big endian. I dont know Ruby but this appears to reverse the bytes when big endian is detected:

def htonq val big_endian? ? ([val].pack("Q").reverse.unpack("Q").first) : val end

It should be the opposite: if the host is big-endian, the bytes are already in network order and should be returned as-is. It is on little endian systems that they should be reversed.

mpalmer commented 11 years ago

Is the problem that I've named the method wrong, or can this actually cause a problem somehow?

nats commented 11 years ago

hton stands for host to network (sorry if you are aware of this). Network order is defined as big endian, so it should only reverse the bytes when the current system is not big endian.

ie. I think the implementation should be flipped:

def htonq val big_endian? ? val : ([val].pack("Q").reverse.unpack("Q").first) : end

As to whether it causes a problem? I think the end result is that in the current implementation you always send the data as little endian, which is fine as long as its your implementation on both ends.

tl;dr; I dont think it will cause a problem, but it was confusing when reading the code.

mpalmer commented 10 years ago

I see what you mean now, and since I'm changing the network protocol version anyway, I may as well fix that now, which I've done in 35ad91f. Thanks for the explanation, and sorry for taking so long to close this off.