BitTorrent peer protocol messages

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
#include
#include

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
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
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
struct variable_payload_message
{
std::ostream& put(std::ostream& os) const;
};

// A template definition for messages, templated on

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 
        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::value, "Only specializations should be used" );, where dependent_false::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 : public variable_payload_message>
        {
        public:
            message() = delete;
            explicit message(std::string bits);

            const std::string payload() const;

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

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

Lately I've been making my traits more convenient to use by adding forwarding specializations: template struct is_message: is_message {};, and another one for const. They help with perfect forwarding because if you have e.g. template 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
        std::ostream& operator::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
std::ostream&
operator const& m);

// simple implementation of operator<< for messages

Change definition to match previous declaration.

// implementation of no-payload message base class
    template 
        std::ostream& no_payload_message::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(&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(&buffer), sizeof buffer) to future-proof (unlikely to matter here but still).

// implementation of fixed-length payload message base class
    template 
        std::ostream& fixed_payload_message::put(std::ostream& os) const {
            auto buffer = static_cast(this)->data(); // get the data from the derived class
            return os.write(static_cast(&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 
        std::ostream& variable_payload_message::put(std::ostream& os) const {
            typedef typename Message::data_type header_type;

            auto m = static_cast(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 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 `ostrea

StackExchange Code Review Q#3770, answer score: 6