Re: Request for Feedback: new BIO API

[Christian Grothoff]

On 5/16/20 12:29 AM, Alessio Vanni wrote:
> Christian Grothoff <address@hidden> 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