Aaron Conole via dev <[email protected]> writes:
> Upcoming commits will be implementing the ChaCha20 high-speed
> symmetric hashing algorithm as part of a CSPRNG for use with
> NAT tuple generation. ChaCha20 treats internal stream data
> as little endian bytes, which means the implementation will
> need a way to read and write byte streams of le32 values.
>
> To facilitate this, try to follow the proposed POSIX endian
> header support as much as possible. Linux and FreeBSD
> differ slightly in how they provide the htole32 and le32toh
> function defintions, but they both provide implementations.
>
> Signed-off-by: Aaron Conole <[email protected]>
> ---
> lib/byte-order.h | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/lib/byte-order.h b/lib/byte-order.h
> index f5a21f3298..c942e78a96 100644
> --- a/lib/byte-order.h
> +++ b/lib/byte-order.h
> @@ -20,6 +20,7 @@
> #include <netinet/in.h>
> #include <arpa/inet.h>
> #include <inttypes.h>
> +#include <string.h>
> #include "openvswitch/types.h"
>
> #ifndef __CHECKER__
> @@ -71,6 +72,39 @@ uint32_byteswap(uint32_t crc) {
> ((crc & 0xff000000) >> 24));
> }
>
> +#ifdef __linux__
> +#include <endian.h>
> +#elif defined(__FreeBSD__)
> +#include <sys/endian.h>
> +#else
> +static inline uint32_t
> +htole32(uint32_t host_32bits)
> +{
> + return htonl(1) == 1 ? uint32_byteswap(host_32bits) : host_32bits;
> +}
> +
> +static inline uint32_t
> +le32toh(uint32_t le_32bits)
> +{
> + return htonl(1) == 1 ? uint32_byteswap(le_32bits) : le_32bits;
> +}
> +#endif
The AI had the following to say about this:
Correctness of the fallback is fine: `htonl(1) == 1` is a
compile-time-foldable endianness test, and `uint32_byteswap` (verified
at lib/byte-order.h:67-73) does a proper full 32-bit swap, so both byte
orders are handled. glibc/musl <endian.h> and FreeBSD <sys/endian.h> do
provide htole32/le32toh, so the primary paths resolve.
Two minor observations, neither function-breaking:
1. OVS officially supports NetBSD
(Documentation/intro/install/netbsd.rst; `__NetBSD__` handled throughout
lib/netdev-bsd.c etc.), and NetBSD also provides htole32/le32toh in
<sys/endian.h>, but the #elif only matches __FreeBSD__. NetBSD therefore
silently gets the local fallback. That is functionally correct, so no
bug; the only hazard would be a translation unit that includes
<sys/endian.h> through some other header before byte-order.h and defines
htole32 as a macro, which would mangle the static-inline fallback
definitions. I could not verify whether such an include path exists in
practice on NetBSD (unverified), and no in-tree header does this today
(grep shows only include/sparse/rte_byteorder.h and lib/byte-order.h
reference endian headers).
2. Defining `htole32`/`le32toh` at global scope in the fallback branch
injects POSIX-namespace identifiers into every consumer of byte-order.h
on non-Linux/non-FreeBSD platforms (e.g., a future libc that adopts the
proposed POSIX <endian.h>, or other third-party headers providing the
same names as macros). Since the commit message itself calls out that
this API is only "proposed POSIX," an `ovs_`-prefixed fallback name
would have been safer. Cosmetic/forward-compat concern only.
I will wait to respin until after a human reviews, but I would make the
following change if someone thinks it is important:
Define ovs_le32toh / ovs_htole32 and update the tests. That way even if
we miss a system that provides an endian header, we wouldn't collide in
the program's main namespace. I don't expect we would anyway, but
better safe than sorry.
Alternatively, maybe we could add a ENDIAN_HEADER check, and it will
give the endian header file location. Then if it isn't defined, we can
define the functions.
> +static inline uint32_t
> +ovs_load_le32(const uint8_t *p)
> +{
> + uint32_t n;
> + memcpy(&n, p, sizeof n);
> + return le32toh(n);
> +}
> +
> +static inline void
> +ovs_store_le32(uint8_t *p, uint32_t v)
> +{
> + uint32_t n = htole32(v);
> + memcpy(p, &n, sizeof n);
> +}
> +
> /* These macros may substitute for htons(), htonl(), and htonll() in contexts
> * where function calls are not allowed, such as case labels. They should
> not
> * be used elsewhere because all of them evaluate their argument many times.
> */
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
