On 5/16/20 12:29 AM, Alessio Vanni wrote: > Christian Grothoff <[email protected]> writes: > >> This looks indeed perfect to me, modulo possibly one issue that is >> totally unrelated to the design and more like a legacy bug you're copying: >> https://en.wikipedia.org/wiki/Endianness#Floating_point >> implies that we should actually also do network byte order conversions >> on double and float, instead of copying them 1:1 into the byte stream. >> But that is an _existing_ bug in BIO I just noticed while reading your >> patch ;-). > > Yeah, I simply copied the I/O on float/double instead of dealing with > endianness. To begin with, since BIO is relatively old, I assumed there > was already some talk about this and I though simply dropping the bytes > as-is was something people agreed upon at some point in time; the other > reason is that changing the underlying representation would break > already-existing serializations and I don't know if that's ok.
I think in this case that's OK, we very, very rarely use doubles in the existing code ;-) -- and we are known to break compatibility from major version to major version in worse ways. > Should I change the float/double API to serialize the numbers into > network byte order? (Hopefully there's a function in libc for this...) Yes. In GNU libc there is a function (ok, Macro) for it: bswap_32() and bswap_64(). But, this is a GNU extension, so for portability we may prefer to brutally use htonl/ntohl/GNUNET_htonll/ntohll: float fh = ...; uint32_t ih = *(uint32_t*) &fh; uint32_t ie = htonl (ih); float fn = *(float*) &ie; >> Otherwise, very nice. The BIO part is IMO ready, but obviously the rest >> of the code needs to be (slightly) adjusted to match the API change >> before we can merge this. > > Great! Then I'll (slowly, since it's rather big) change the rest of the > codebase to account for the new API, then reply back to this > conversation with the full patch (tests included of course.) :-) -Christian
signature.asc
Description: OpenPGP digital signature
