1
\$\begingroup\$

Context:

When sending message across the network, the sender has to convert the message from host byte order to network byte order and the receiver has to do the the reverse - network order to host order.

There are multiple ways of going about it. One neat way to do is to convert the message to host byte order as soon as it is received.

Eg:

struct Msg {
   uint32_t a;
   uint16_t b;
};

run_ntoh (Msg *m)
{
    m->a = ntohl(m->a);
    m->b = ntohs(m->b);
}

process (Msg *m)
{
   run_ntoh(m);
   further_process(m);   
}

But there are cases where this will not work. Say, the message buffer is shared by multiple receivers. We can always copy the message to local buffer and do it. But that is inefficient if the message is large.

The third option is to always access the fields of 'Msg' using ntohx function. But this is error prone.

a. The user might forget to use ntoh always - it is not enforced.

b. Also user has to be very careful - use htonl or htons? What if the size of the varible is changed - nothing enforces the change of htonl instead of htons if the variable size changes from 16 to 32.

Here my idea is to enforce the use of ntoh, while making it easy for the user.

Code:

#include <cstdint>
#include <cstddef>
#include <type_traits>

//
// class to extract N number of bytes and form a number
// uint32_t value = ExtractValue<uint32_t>::get(buffer);
//
template <typename T, size_t val = sizeof(T)>
struct ExtractValue {
    static T get (uint8_t *p)
    {
        return (ExtractValue<T, val - 1>::get(p) << 8) |
                static_cast<T>(p[val - 1]);
    }
};

template <typename T>
struct ExtractValue<T, 0> {
    static T get (uint8_t *p)
    {
        return 0;
    }
};

// Get a field of the class T, in host byte order.
// The class T is wrapped in NetworkData class. The get() function
// takes the offset to the member as an argument, gets the address
// of field and extract the field
template <typename T>
struct NetworkData {

    using type = T;

    NetworkData (uint8_t *p) : _p(p) {}

    template<typename R, typename U = std::decay_t<R>,
             typename std::enable_if_t<!std::is_pointer<U>::value>* =  nullptr>
    U get (uintptr_t offset)
    {
        return ExtractValue<U>::get(_p + offset);
    }

    template<typename R, typename U = std::decay_t<R>,
             typename std::enable_if_t<std::is_pointer<U>::value>* = nullptr>
    U get (uintptr_t offset)
    {
        return (U)(_p + offset);
    }

private:
    uint8_t *_p;
};

// Macro to easily call the NetworkData::get()
#define GetNtoh(h, f)  \
    h.get<decltype(decltype(h)::type::f)>(offsetof(decltype(h)::type, f))

Usage:

struct Msg {
   uint32_t a;
   uint16_t b;
};

void process (uint8_t *p)
{
    NetworkData<Msg> h(p);

    auto a = GetNtoh(h, a);
    auto b = GetNtoh(h, b);
}

It is always better to use something like goolge protobuf to serialize/deserialize. But that is not possible when the sender is legacy code.

\$\endgroup\$
  • \$\begingroup\$ What's the difference of the macros HTONGET() and NTOHGET()? \$\endgroup\$ – πάντα ῥεῖ Mar 29 '17 at 17:53
  • 1
    \$\begingroup\$ @πάνταῥεῖ The difference is in the name. NetworkToHost and HostToNetwrok are generally mirrors or NOP. \$\endgroup\$ – Stephen Rauch Mar 29 '17 at 18:23
  • 1
    \$\begingroup\$ Welcome to StackExchange Code Review! Please review How do I ask a good Question? Specifically, you will get more insightful reviews if you not only provide your code, but also give an explanation of what it does. The more detail, the better. \$\endgroup\$ – Stephen Rauch Mar 29 '17 at 18:24
  • 1
    \$\begingroup\$ What are: hton16 hton32 hton64? I always though it was htons and htonl? \$\endgroup\$ – Martin York Mar 29 '17 at 18:53
  • 1
    \$\begingroup\$ @loki There seem to be such definitions in 3rd party libs: sourceforge.net/p/cplusplus/libzrtpcpp/ci/… \$\endgroup\$ – πάντα ῥεῖ Mar 29 '17 at 18:58
0
\$\begingroup\$

Unqualified names from C headers

It appears that your <cstdint> happens to declare its identifiers in the global namespace as well as in std; this means that you weren't notified of the consistent misspelling of std::uint8_t, std::uint16_t and the like.

Interface

It's not clear why ExtractValue is visible at file scope (rather than being a private static method within NetworkData).

The get() methods ought to be declared const (and if you never implement set() methods, then perhaps _p should be a pointer-to-const).

Pass pointer to member rather than offsetof

The use of offsetof() and pointer arithmetic with std::uint8_t* (which should be char*, given that offsetof() works in units of char) is deeply disturbing. One possibility is for NetworkData<T> to hold a private reference to the T, and accept pointer-to-member to indicate which value is desired:

#include <type_traits>
#include <utility>

// Get a field of the class T, in host byte order.
// The class T is wrapped in NetworkData class. The get() function
// takes the offset to the member as an argument, gets the address
// of field and extract the field
template <typename T>
class NetworkData
{
    const T& data;

public:
    NetworkData (void *data)
        : data(*reinterpret_cast<T*>(data))
    {
    }

    template<typename V>
    V get(V T::*field) const
    {
#if BIG_ENDIAN
        return data.*field;
#else
        return byte_swap(data.*field);
#endif
    }

private:
    template<typename V>
    static
    std::enable_if_t<std::is_integral_v<V> && std::is_signed_v<V>, V>
    byte_swap(V value)
    {
        // We manipulate using unsigned bitwise arithmetic.
        return reinterpret_cast<V>(byte_swap(reinterpret_cast<std::make_unsigned_t<V>>(value)));
    }

    template<typename V>
    static
    std::enable_if_t<std::is_integral_v<V> && std::is_unsigned_v<V>, V>
    byte_swap(V value)
    {
        // We assume CHAR_BIT == 8 (so that sizeof gives us octets)
        static const V mask = 0xff;
        V result = 0;
        for (auto shift = 0u;  shift < 8 * sizeof value;  shift += 8)
            result = (result << 8) + ((value >> shift) & mask);
        return result;
    }

    // We could add overloads here to take advantage of processor-native byte swap instructions, as available.
};

We can then use it like this:

#include <cstdint>
struct Msg {
   std::uint32_t a;
   std::uint16_t b;
};

void process (std::uint8_t *p)
{
    NetworkData<Msg> h(p);

    auto a = h.get(&Msg::a);
    auto b = h.get(&Msg::b);
}

This eliminates the use of preprocessor macros, and makes a slightly more natural object-access syntax.

Consider modifying the structure members

If we're allowed to re-write Msg, we could make its members be network-endian values:

#include <cstdint>
struct Msg {
   NetworkEndian<std::uint32_t> a;
   NetworkEndian<std::uint16_t> b;
};

We can declare NetworkEndian like this:

template<typename Integer>
class NetworkEndian
{
    Integer value;

public:
    NetworkEndian(Integer value)
        : value(byte_swap(value))
    {}
    // default copy, assign, destructor

    operator Integer() const
    {
        return byte_swap(value);
    }

private:
    static Integer byte_swap(Integer v)
    {
        using Unsigned = std::make_unsigned_t<Integer>;
        Unsigned u = reinterpret_cast<Unsigned>(v);
        // Assume CHAR_BIT == 8 (so that sizeof gives us octets)
        static const Unsigned mask = 0xff;
        Unsigned result = 0;
        for (auto shift = 0u;  shift < 8 * sizeof u;  shift += 8)
            result = (result << 8) + ((u >> shift) & mask);
        return reinterpret_cast<Integer>(result);
    }
};

The conversion operators mean that we can now read the members in a completely natural manner:

void process(const Msg& message)
{
    auto a = message.a;
    auto b = message.b;
}

Not only that, but by providing a converting constructor, we also get to assign values equally naturally.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments. Couple of things 1: NetworkData (void *data) : data(*reinterpret_cast<T*>(data))' - there is no alignment guarantee of 'void * data'. So type casting it to T * is not strictly valid. But I haven't mentioned that in the original question. But that's the reason for the gimmick with the offsetof - which I think is okay given we are dealing with POD types. Also we want avoid memcpy to a aligned location. If we can do the memcpy, the need for this interface goes away, as we can deserilize it such that all fields are converted in one shot. \$\endgroup\$ – MGH Oct 27 '17 at 14:07
  • \$\begingroup\$ Second thing changing the Msg structure is not possible - it is shared with legacy C code. But the code you shared will be useful in other context. Also thanks for the comments one std::uint.. , get should be const - yes. Extract value is in file scope as it is a generic class to extract an integral type T from a unaligned buffer. \$\endgroup\$ – MGH Oct 27 '17 at 14:13
  • \$\begingroup\$ Yes, I did think that the definition of Msg might be externally-imposed; that's why I put that section at the end, and prefaced it with "if" - I hope that gives some insight, anyway. And you're right that I hadn't considered alignment - I admit that I had assumed that the caller had arranged that. (P.S. if you found my answer helpful, you may vote using the ^ button; wait a while for any other answers before choosing one to accept). \$\endgroup\$ – Toby Speight Oct 27 '17 at 14:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.