BitTorrent peer protocol messages

Question

I'm writing a BitTorrent client. Part of the protocol is an exchange of length-prefixed messages between peers. The forms of these messages are described in the official specification and also in the unofficial specification that is maintained by the community. This is my code for writing out this messages into a stream. I'm currently using C++11 as supported by GCC 4.6.

I have a header file:

#ifndef MESSAGE_HPP
#define MESSAGE_HPP

#include <ostream>
#include <string>
#include <type_traits>

namespace greed {
    // This type defines all the message types and the matching codes
    // It uses C++11's ability to define an enum's underlying type
    enum message_type : signed char
    {
    // this keep_alive as negative is a hack I'm not too happy with
        keep_alive      = -1,
        choke           = 0,
        unchoke         = 1,
        interested      = 2,
        not_interested  = 3,
        have            = 4,
        bitfield        = 5,
        request         = 6,
        piece           = 7,
        cancel          = 8
    };

    // There are three basic kinds of messages
    // These three basic templates are used in CRTP below
    //  - messages with no payload: these contain only the length
    //         and the type code
    template <message_type Type>
        struct no_payload_message
        {
            std::ostream& put(std::ostream& os) const;
        };

    //  - messages with a fixed-length payload: these contain the length,
    //         the type code, and a payload of a fixed-length, that is
    //         determined by the message type.
    template <typename Message>
        struct fixed_payload_message
        {
            std::ostream& put(std::ostream& os) const;
        };

    //  - messages with a payload of variable length: these contain the length,
    //         the type code, and the payload.
    template <typename Message>
        struct variable_payload_message
        {
            std::ostream& put(std::ostream& os) const;
        };

    // A template definition for messages, templated on the message type.
    template <message_type Type>
        struct message {};

    // A specialization for keep-alives, which are special messages that
    // consist of a single zero byte.
    template<>
        struct message<keep_alive>
        {
        public:
            std::ostream& put(std::ostream& os) const;
        };

    // Specializations for the no-payload messages types
    // The only difference between these is the message type.
    template <>
        struct message<choke> : public no_payload_message<choke> {};

    template <>
        struct message<unchoke> : public no_payload_message<unchoke> {};

    template <>
        struct message<interested> : public no_payload_message<interested> {};

    template <>
        struct message<not_interested> : public no_payload_message<not_interested> {};

    // The specializations for fixed-length payload messages contain:
    //  - appropriate constructors
    //  - a type that defines the format of the message (data_type)
    //  - a function that returns the data (data)
    // These are used by the fixed_payload_message template
    template<>
        struct message<have> : public fixed_payload_message<message<have>>
        {
        public:
            message() = delete;
            explicit message(unsigned index);

            struct data_type;
            data_type data() const;

        private:
            unsigned index;
        };

    // The specializations for variable-length payload messages contain:
    //  - appropriate constructors
    //  - a type that defines the format of the message (data_type)
    //  - a function that returns the variable payload (payload)
    //  - a function that writes out the parts of the message that are not variable (init)
    // These are used by the variable_payload_message template
    template<>
        struct message<bitfield> : public variable_payload_message<message<bitfield>>
        {
        public:
            message() = delete;
            explicit message(std::string bits);

            const std::string payload() const;

            struct data_type;
            void init(char* ptr) const;

        private:
            std::string bits;
        };

    template<>
        struct message<request> : public fixed_payload_message<message<request>>
        {
        public:
            message() = delete;
            message(unsigned index, unsigned begin, unsigned length);

            struct data_type;
            data_type data() const;

        private:
            unsigned index;
            unsigned begin;
            unsigned length;
        };

    template<>
        struct message<piece> : public variable_payload_message<message<piece>>
        {
        public:
            static const message_type id = piece;

            message() = delete;
            message(unsigned index, unsigned begin, std::string block);

            const std::string payload() const;

            struct data_type;
            void init(char* ptr) const;

        private:
            unsigned index;
            unsigned begin;
            std::string block;
        };

    template<>
        struct message<cancel> : public fixed_payload_message<message<cancel>>
        {
        public:
            message() = delete;
            message(unsigned index, unsigned begin, unsigned length);

            struct data_type;
            data_type data() const;

        private:
            unsigned index;
            unsigned begin;
            unsigned length;
        };

    // A simple type trait that determines if a type is a message type
    template <typename NonMesssage>
        struct is_message : std::false_type {};
    template <message_type Type>
        struct is_message<message<Type>> : std::true_type {};

    // Implementation of operator<< for all message types
    // This requires the put member function
    template <typename Message>
        std::ostream& operator<<(std::ostream& os, typename std::enable_if<is_message<Message>::value,const Message&>::type message);
}

#endif

And an implementation file:

#include "message.hpp"
// this header contains the hton function
#include "util.hpp"

#include <memory>
#include <new>

namespace greed {
    // simple implementation of operator<< for messages
    template <typename Message>
        std::ostream& operator<<(std::ostream& os, const Message& message) {
            return message.put(os);
        }

    // implementation of no-payload message base class
    template <message_type Type>
        std::ostream& no_payload_message<Type>::put(std::ostream& os) const {
            // make sure this struct is not padded or anything
            // this is a gcc attribute, I'll change this when there is
            // support for the C++11 attribute alignas
            struct __attribute__ ((packed)) data_type {
                unsigned len;
                message_type type;
            };
            // hton is a function that converts from host-endianness to network-endianness
            data_type buffer = { hton(sizeof(data_type)-sizeof(data_type::len)), Type }; // lay out the length and the message type
            return os.write(reinterpret_cast<char*>(&buffer), sizeof(data_type)); // write it
        }

    // implementation of fixed-length payload message base class
    template <typename Message>
        std::ostream& fixed_payload_message<Message>::put(std::ostream& os) const {
            auto buffer = static_cast<const Message*>(this)->data(); // get the data from the derived class
            return os.write(reinterpret_cast<char*>(&buffer), sizeof(buffer)); // write it
        }

    // implementation of variable-length payload message base class
    template <typename Message>
        std::ostream& variable_payload_message<Message>::put(std::ostream& os) const {
            typedef typename Message::data_type header_type;

            auto m = static_cast<const Message*>(this);
            const auto payload = m->payload(); // get the payload
            const auto data_type_size = sizeof(header_type)+payload.size(); // get the total size
            std::unique_ptr<char[]> mem(new char[data_type_size]); // allocate a buffer for it
            m->init(mem.get()); // write out the fixed-length portion
            std::copy(payload.begin(), payload.end(), mem.get()+sizeof(header_type)); // copy the payload to the buffer
            return os.write(mem.get(), data_type_size); // write it
        }

    std::ostream& message<keep_alive>::put(std::ostream& os) const {
        return os.put(0); // keep-alives are just a simple zero byte
    }

    message<have>::message(unsigned index) : index(index) {}
    struct __attribute__ ((packed)) message<have>::data_type{
        unsigned len;
        message_type type;
        unsigned index;
    };
    // have message data
    message<have>::data_type message<have>::data() const {
        return data_type{ hton(sizeof(data_type)-sizeof(data_type::len)), have, hton(index) };
    }

    message<bitfield>::message(std::string bits) : bits(bits) {}
    struct __attribute__ ((packed)) message<bitfield>::data_type {
        unsigned len;
        message_type type;
    };
    // bitfield message payload
    const std::string message<bitfield>::payload() const {
        return bits;
    }
    void message<bitfield>::init(char* ptr) const {
        // construct a new message<bitfield>::data_type in place
        new(ptr) data_type{ hton(sizeof(data_type)-sizeof(data_type::len)+bits.size()), bitfield };
    }

    message<request>::message(unsigned index, unsigned begin, unsigned length) : index(index), begin(begin), length(length) {}
    struct __attribute__ ((packed)) message<request>::data_type {
        unsigned len;
        message_type type;
        unsigned index;
        unsigned begin;
        unsigned length;
    };
    // request message data
    message<request>::data_type message<request>::data() const {
        return data_type{ hton(sizeof(data_type)-sizeof(data_type::len)), request, hton(index), hton(begin), hton(length) };
    }

    message<piece>::message(unsigned index, unsigned begin, std::string block) : index(index), begin(begin), block(block) {}
    struct __attribute__ ((packed)) message<piece>::data_type {
        unsigned len;
        message_type type;
        unsigned index;
        unsigned begin;
    };
    const std::string message<piece>::payload() const {
        return block;
    }
    void message<piece>::init(void* ptr) const {
        // construct a new message<piece>::data_type in place
        new(ptr) data_type{ hton(sizeof(data_type)-sizeof(data_type::len)+block.size()), piece, index, begin };
    }

    message<cancel>::message(unsigned index, unsigned begin, unsigned length) : index(index), begin(begin), length(length) {}
    struct __attribute__ ((packed)) message<cancel>::data_type {
        unsigned len;
        message_type type;
        unsigned index;
        unsigned begin;
        unsigned length;
    };
    // cancel message data
    message<cancel>::data_type message<cancel>::data() const {
        return data_type{ hton(sizeof(data_type)-sizeof(data_type::len)), cancel, hton(index), hton(begin), hton(length) };
    }

    // explicit template instantiations for all message types
    template struct message<keep_alive>;
    template struct message<choke>;
    template struct message<unchoke>;
    template struct message<interested>;
    template struct message<not_interested>;
    template struct message<have>;
    template struct message<bitfield>;
    template struct message<request>;
    template struct message<piece>;
    template struct message<cancel>;

    template std::ostream& operator<<(std::ostream& os, const message<keep_alive>& message);
    template std::ostream& operator<<(std::ostream& os, const message<choke>& message);
    template std::ostream& operator<<(std::ostream& os, const message<unchoke>& message);
    template std::ostream& operator<<(std::ostream& os, const message<interested>& message);
    template std::ostream& operator<<(std::ostream& os, const message<not_interested>& message);
    template std::ostream& operator<<(std::ostream& os, const message<have>& message);
    template std::ostream& operator<<(std::ostream& os, const message<bitfield>& message);
    template std::ostream& operator<<(std::ostream& os, const message<request>& message);
    template std::ostream& operator<<(std::ostream& os, const message<piece>& message);
    template std::ostream& operator<<(std::ostream& os, const message<cancel>& message);
}

I'm a bit unsure about how I dealt with the variable-length messages, especially the usage of placement new, without calling the destructor.

So, opinions?

Answer 1

    enum message_type : signed char

Do you have a specific reason (e.g. protocol requirements?) that this be signed? Bit-level stuff usually involves unsigned types. The protocol specs also explicity mention the size of the message type field: a single byte. So I recommend std::uint8_t here.

    // A template definition for messages, templated on the message type.
    template <message_type Type>
        struct message {};

If only the specializations are meant to be used (I couldn't tell from looking at your code), I usually 'forbid' the base template to catch mistakes early. Errors about how message<...> has no put member are confusing and not necessarily near the code that instantiated the template. The simplest way to do it is to leave the template undefined but lately I've been using a trick: static_assert( dependent_false<Type>::value, "Only specializations should be used" );, where dependent_false<T>::value is always false but won't trigger the assert until instantiation (whereas static_assert( false, ... ) always triggers and won't let you compile, ever.)

    template<>
        struct message<bitfield> : public variable_payload_message<message<bitfield>>
        {
        public:
            message() = delete;
            explicit message(std::string bits);

            const std::string payload() const;

I'd prefer returning std::string here. (Ditto for message<piege>::payload.)

    // A simple type trait that determines if a type is a message type
    template <typename NonMesssage>
        struct is_message : std::false_type {};
    template <message_type Type>
        struct is_message<message<Type>> : std::true_type {};

Lately I've been making my traits more convenient to use by adding forwarding specializations: template<typename T> struct is_message<T&>: is_message<T> {};, and another one for const. They help with perfect forwarding because if you have e.g. template<typename M> void perfectly_forwarded(M&&); then M might be T const& for some T. Since you're not doing that in your code I don't think you need it -- just a head's up.

    // Implementation of operator<< for all message types
    // This requires the put member function
    template <typename Message>
        std::ostream& operator<<(std::ostream& os, typename std::enable_if<is_message<Message>::value,const Message&>::type message);

Deduction can't work here. C++03-style code uses enable_if at the return type, or when there is no return type (e.g. constructors) as a default argument. Maybe you tried to 'collapse' the default argument with the actual, interesting parameter, but you can't do that. C++0x-style code can put enable_if as a defaulted template parameters but that's moot since you really want (credit to CatPlusPlus):

template<message_type M>
std::ostream&
operator<<(std::ostream& os, message<M> const& m);

---

    // simple implementation of operator<< for messages

Change definition to match previous declaration.

    // implementation of no-payload message base class
    template <message_type Type>
        std::ostream& no_payload_message<Type>::put(std::ostream& os) const {
            // make sure this struct is not padded or anything
            // this is a gcc attribute, I'll change this when there is
            // support for the C++11 attribute alignas
            struct __attribute__ ((packed)) data_type {
                unsigned len;
                message_type type;
            };
            // hton is a function that converts from host-endianness to network-endianness
            data_type buffer = { hton(sizeof(data_type)-sizeof(data_type::len)), Type }; // lay out the length and the message type
            return os.write(static_cast<char*>(&buffer), sizeof(data_type)); // write it
        }

Again, following protocol specs, I'd make len a std::uint32_t. The unofficial spec use 1 as the length, I'm not sure what you're computing here. Minor note: I always write os.write(static_cast<char*>(&buffer), sizeof buffer) to future-proof (unlikely to matter here but still).

    // implementation of fixed-length payload message base class
    template <typename Message>
        std::ostream& fixed_payload_message<Message>::put(std::ostream& os) const {
            auto buffer = static_cast<const Message*>(this)->data(); // get the data from the derived class
            return os.write(static_cast<char*>(&buffer), sizeof(buffer)); // write it
        }

And in fact here you do take the size of the object!

    // implementation of variable-length payload message base class
    template <typename Message>
        std::ostream& variable_payload_message<Message>::put(std::ostream& os) const {
            typedef typename Message::data_type header_type;

            auto m = static_cast<const Message*>(this);
            const auto payload = m->payload(); // get the payload
            const auto data_type_size = sizeof(header_type)+payload.size(); // get the total size
            std::unique_ptr<char[]> mem(new char[data_type_size]); // allocate a buffer for it
            m->init(mem.get()); // write out the fixed-length portion
            std::copy(payload.begin(), payload.end(), mem.get()+sizeof(header_type)); // copy the payload to the buffer
            return os.write(mem.get(), data_type_size); // write it
        }

I don't see the need to copy the final message into a buffer. Why not use two calls to ostream::write? You could replace the init members (which you already dislike) with a data member like the fixed-length messages.

// Lots of definitions/instantiations

Again, I didn't check conformance to the specs. I also already mentioned to you how most of the explicit instantiations aren't needed.

I'd also write sizeof field0 + sizeof field1 + ... for computing the various sizes that you need rather than subtracting from the total size. I'd do that for clarity and I don't think it's a correctness issue however.

---

Final remarks on the general design:

I think you're somewhat abusing specializations here: there's is no need (IMO) for a message catch-all template since the three kinds of messages are not similar in use. This really shows I think with the constructors that aren't compatible: you can't write a generic message<M> m(arguments go here);. Personally I'd have used overloaded function templates to return those three types.

Also I personally typically use std::vector<unsigned char> for binary stuff rather than std::string but I don't think that really matters (plus you may have a use for some std::string-specific stuff that is not in the code you presented).

Answer 2

Can't really complain about most of it.

The only thing I dislike is the abuse of auto:

        auto m = static_cast<const Message*>(this);
        const auto payload = m->payload(); // get the payload
        const auto data_type_size = sizeof(header_type)+payload.size(); // get the total size

You know what the types are so why not be specific (personally I think that will make the code easier to read).