gnunet-developers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Request for Feedback: new BIO API


From: Christian Grothoff
Subject: Re: Request for Feedback: new BIO API
Date: Sun, 17 May 2020 16:48:02 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]