Remove read/write_port and comment proto::packet.

src/proto/packet.rs

@@ -3,7 +3,7 @@ use std::fmt;
 use std::io;
 use std::mem;
 use std::net;
-use std::iter::repeat;
+use std::iter;
 use std::io::{Read, Write};
 
 use byteorder::{ByteOrder, LittleEndian, ReadBytesExt, WriteBytesExt};
@@ -51,6 +51,7 @@ impl io::Write for Packet {
 }
 
 impl Packet {
+    /// Returns an empty packet with the given packet code.
     pub fn new(code: u32) -> Self {
         let mut bytes = Vec::new();
         bytes.write_u32::<LittleEndian>(0).unwrap();
@@ -61,6 +62,8 @@ impl Packet {
         }
     }
 
+    /// Returns a new packet struct, constructed from the wire representation
+    /// of a packet.
     fn from_raw_parts(bytes: Vec<u8>) -> Self {
         let size = LittleEndian::read_u32(&bytes[..U32_SIZE]) as usize;
         assert!(size + U32_SIZE == bytes.len());
@@ -70,22 +73,6 @@ impl Packet {
         }
     }
 
-    // Writing convenience
-
-    pub fn write_port(&mut self, port: u16) -> io::Result<()> {
-        self.write_value(port as u32)
-    }
-
-    /// This function is necessary because not all u16 values are encoded in
-    /// 4 bytes.
-    pub fn read_port(&mut self) -> Result<u16, PacketReadError> {
-        let port: u32 = try!(self.read_value());
-        if port > MAX_PORT {
-            return Err(PacketReadError::InvalidPortError(port))
-        }
-        Ok(port as u16)
-    }
-
     /// Provides the main way to read data out of a binary packet.
     pub fn read_value<T: ReadFromPacket>(&mut self)
         -> Result<T, PacketReadError>
@@ -105,7 +92,8 @@ impl Packet {
         self.bytes.len() - self.cursor
     }
 
-
+    /// Returns a slice pointing to the entire underlying byte array, including
+    /// the length prefix.
     pub fn as_slice(&mut self) -> &[u8] {
         let bytes_len = (self.bytes.len() - U32_SIZE) as u32;
         {
@@ -120,12 +108,20 @@ impl Packet {
  * PACKET READ ERROR *
  *===================*/
 
+/// This enum contains an error that arose when reading data out of a Packet.
 #[derive(Debug)]
 pub enum PacketReadError {
+    /// Attempted to read a boolean, but the value was not 0 nor 1.
     InvalidBoolError(u8),
-    InvalidPortError(u32),
+    /// Attempted to read an unsigned 16-bit integer, but the value was too
+    /// large.
+    InvalidU16Error(u32),
+    /// Attempted to read a string, but a character was invalid.
     InvalidStringError(Vec<u8>),
+    /// Attempted to read a user::Status, but the value was not a valid
+    /// representation of an enum variant.
     InvalidUserStatusError(u32),
+    /// Encountered an I/O error while reading.
     IOError(io::Error),
 }
 
@@ -134,8 +130,8 @@ impl fmt::Display for PacketReadError {
         match *self {
             PacketReadError::InvalidBoolError(n) =>
                 write!(fmt, "InvalidBoolError: {}", n),
-            PacketReadError::InvalidPortError(n) =>
-                write!(fmt, "InvalidPortError: {}", n),
+            PacketReadError::InvalidU16Error(n) =>
+                write!(fmt, "InvalidU16Error: {}", n),
             PacketReadError::InvalidStringError(ref bytes) =>
                 write!(fmt, "InvalidStringError: {:?}", bytes),
             PacketReadError::InvalidUserStatusError(n) =>
@@ -151,8 +147,8 @@ impl error::Error for PacketReadError {
         match *self {
             PacketReadError::InvalidBoolError(_) =>
                 "InvalidBoolError",
-            PacketReadError::InvalidPortError(_) =>
-                "InvalidPortError",
+            PacketReadError::InvalidU16Error(_) =>
+                "InvalidU16Error",
             PacketReadError::InvalidStringError(_) =>
                 "InvalidStringError",
             PacketReadError::InvalidUserStatusError(_) =>
@@ -165,7 +161,7 @@ impl error::Error for PacketReadError {
     fn cause(&self) -> Option<&error::Error> {
         match *self {
             PacketReadError::InvalidBoolError(_)       => None,
-            PacketReadError::InvalidPortError(_)       => None,
+            PacketReadError::InvalidU16Error(_)        => None,
             PacketReadError::InvalidStringError(_)     => None,
             PacketReadError::InvalidUserStatusError(_) => None,
             PacketReadError::IOError(ref err)          => Some(err),
@@ -183,16 +179,20 @@ impl From<io::Error> for PacketReadError {
  * READ FROM PACKET *
  *==================*/
 
+/// This trait is implemented by types that can be deserialized from binary
+/// Packets.
 pub trait ReadFromPacket: Sized {
     fn read_from_packet(&mut Packet) -> Result<Self, PacketReadError>;
 }
 
+/// 32-bit integers are serialized in 4 bytes, little-endian.
 impl ReadFromPacket for u32 {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         packet.read_u32::<LittleEndian>().map_err(PacketReadError::from)
     }
 }
 
+/// For convenience, usize's are deserialized as u32's then casted.
 impl ReadFromPacket for usize {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let n: u32 = try!(packet.read_value());
@@ -200,6 +200,7 @@ impl ReadFromPacket for usize {
     }
 }
 
+/// Booleans are serialized as single bytes, containing either 0 or 1.
 impl ReadFromPacket for bool {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let mut buffer = vec![0];
@@ -212,6 +213,18 @@ impl ReadFromPacket for bool {
     }
 }
 
+/// 16-bit integers are serialized as 32-bit integers.
+impl ReadFromPacket for u16 {
+    fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
+        let n: u32 = try!(packet.read_value());
+        if n > MAX_PORT {
+            return Err(PacketReadError::InvalidU16Error(n))
+        }
+        Ok(n as u16)
+    }
+}
+
+/// IPv4 addresses are serialized directly as 32-bit integers.
 impl ReadFromPacket for net::Ipv4Addr {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let ip: u32 = try!(packet.read_value());
@@ -219,6 +232,8 @@ impl ReadFromPacket for net::Ipv4Addr {
     }
 }
 
+/// Strings are serialized as length-prefixed arrays of ISO-8859-1 encoded
+/// characters.
 impl ReadFromPacket for String {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let len = try!(packet.read_value());
@@ -231,6 +246,7 @@ impl ReadFromPacket for String {
     }
 }
 
+/// Vectors are serialized as length-prefixed arrays of values.
 impl<T: ReadFromPacket> ReadFromPacket for Vec<T> {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let len: usize = try!(packet.read_value());
@@ -246,16 +262,20 @@ impl<T: ReadFromPacket> ReadFromPacket for Vec<T> {
  * WRITE TO PACKET *
  *=================*/
 
-pub trait WriteToPacket: Sized {
+/// This trait is implemented by types that can be serialized to a binary
+/// Packet.
+pub trait WriteToPacket {
     fn write_to_packet(self, &mut Packet) -> io::Result<()>;
 }
 
+/// 32-bit integers are serialized in 4 bytes, little-endian.
 impl WriteToPacket for u32 {
     fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
         packet.write_u32::<LittleEndian>(self)
     }
 }
 
+/// Booleans are serialized as single bytes, containing either 0 or 1.
 impl WriteToPacket for bool {
     fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
         try!(packet.write(&[self as u8]));
@@ -263,6 +283,15 @@ impl WriteToPacket for bool {
     }
 }
 
+/// 16-bit integers are serialized as 32-bit integers.
+impl WriteToPacket for u16 {
+    fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
+        (self as u32).write_to_packet(packet)
+    }
+}
+
+/// Strings are serialized as a length-prefixed array of ISO-8859-1 encoded
+/// characters.
 impl<'a> WriteToPacket for &'a str {
     fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
         let bytes = match ISO_8859_1.encode(self, EncoderTrap::Strict) {
@@ -278,6 +307,7 @@ impl<'a> WriteToPacket for &'a str {
     }
 }
 
+/// Deref coercion does not happen for trait methods apparently.
 impl<'a> WriteToPacket for &'a String {
     fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
         packet.write_value::<&'a str>(self)
@@ -312,6 +342,7 @@ pub struct PacketStream<T: Read + Write + Evented> {
 
 impl<T: Read + Write + Evented> PacketStream<T> {
 
+    /// Returns a new PacketStream wrapping the provided byte stream.
     pub fn new(stream: T) -> Self {
         PacketStream {
             stream: stream,
@@ -321,7 +352,18 @@ impl<T: Read + Write + Evented> PacketStream<T> {
         }
     }
 
+    /// Attemps to read a packet in a non-blocking fashion.
+    /// If enough bytes can be read from the underlying byte stream to form a
+    /// complete packet `p`, returns `Ok(Some(p))`.
+    /// If not enough bytes are available, returns `Ok(None)`.
+    /// If an I/O error `e` arises when trying to read the underlying stream,
+    /// returns `Err(e)`.
+    /// Note: as long as this function returns `Ok(Some(p))`, the caller is
+    /// responsible for calling it once more to ensure that all packets are
+    /// read as soon as possible.
     pub fn try_read(&mut self) -> io::Result<Option<Packet>> {
+        // Try to read as many bytes as we currently need from the underlying
+        // byte stream.
         let offset = self.buffer.len() - self.num_bytes_left;
         match try!(self.stream.try_read(&mut self.buffer[offset..])) {
             None => (),
@@ -332,12 +374,17 @@ impl<T: Read + Write + Evented> PacketStream<T> {
             },
         }
 
+        // If we haven't read enough bytes, return.
         if self.num_bytes_left > 0 {
             return Ok(None);
         }
 
+        // Otherwise, the behavior depends on what state we were in.
         match self.state {
             State::ReadingLength => {
+                // If we have finished reading the length prefix, then
+                // deserialize it, switch states and try to read the packet
+                // bytes.
                 let message_len =
                     LittleEndian::read_u32(&mut self.buffer) as usize;
                 if message_len > MAX_MESSAGE_SIZE {
@@ -345,11 +392,13 @@ impl<T: Read + Write + Evented> PacketStream<T> {
                 };
                 self.state = State::ReadingPacket;
                 self.num_bytes_left = message_len;
-                self.buffer.extend(repeat(0).take(message_len));
+                self.buffer.extend(iter::repeat(0).take(message_len));
                 self.try_read()
             },
 
             State::ReadingPacket => {
+                // If we have finished reading the packet, swap the full buffer
+                // out and return the packet made from the full buffer.
                 self.state = State::ReadingLength;
                 self.num_bytes_left = U32_SIZE;
                 let new_buffer = vec![0;U32_SIZE];
@@ -359,6 +408,10 @@ impl<T: Read + Write + Evented> PacketStream<T> {
         }
     }
 
+    /// Tries to write a given packet to the underlying byte stream.
+    /// TODO: If the packet is not entirely written in the first call, this
+    /// will send garbage along the wire. Instead we should track how far we
+    /// are in sending the given packet?
     pub fn try_write(&mut self, packet: &mut Packet) -> io::Result<Option<()>> {
         match try!(self.stream.try_write(packet.as_slice())) {
             None => Ok(None),
@@ -366,6 +419,7 @@ impl<T: Read + Write + Evented> PacketStream<T> {
         }
     }
 
+    /// Register the packet stream with the given mio event loop.
     pub fn register<U: Handler>(
         &self, event_loop: &mut EventLoop<U>, token: Token,
         event_set: EventSet, poll_opt: PollOpt)
@@ -374,6 +428,7 @@ impl<T: Read + Write + Evented> PacketStream<T> {
         event_loop.register(&self.stream, token, event_set, poll_opt)
     }
 
+    /// Re-register the packet stream with the given mio event loop.
     pub fn reregister<U: Handler>(
         &self, event_loop: &mut EventLoop<U>, token: Token,
         event_set: EventSet, poll_opt: PollOpt)

src/proto/server/request.rs

@@ -188,7 +188,7 @@ pub struct SetListenPortRequest {
 
 impl<'a> WriteToPacket for &'a SetListenPortRequest {
     fn write_to_packet(self, packet: &mut Packet) -> io::Result<()> {
-        try!(packet.write_port(self.port));
+        try!(packet.write_value(self.port));
         Ok(())
     }
 }

src/proto/server/response.rs

@@ -106,32 +106,30 @@ impl ReadFromPacket for ServerResponse {
 
 #[derive(Debug)]
 pub struct ConnectToPeerResponse {
-    pub username: String,
+    pub username:        String,
     pub connection_type: String,
-    pub ip: net::Ipv4Addr,
-    pub port: u16,
-    pub token: u32,
-    pub is_privileged: bool,
+    pub ip:              net::Ipv4Addr,
+    pub port:            u16,
+    pub token:           u32,
+    pub is_privileged:   bool,
 }
 
 impl ReadFromPacket for ConnectToPeerResponse {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
-        let username = try!(packet.read_value());
+        let username        = try!(packet.read_value());
         let connection_type = try!(packet.read_value());
-
-        let ip = try!(packet.read_value());
-        let port = try!(packet.read_port());
-
-        let token = try!(packet.read_value());
-        let is_privileged = try!(packet.read_value());
+        let ip              = try!(packet.read_value());
+        let port            = try!(packet.read_value());
+        let token           = try!(packet.read_value());
+        let is_privileged   = try!(packet.read_value());
 
         Ok(ConnectToPeerResponse {
-            username: username,
+            username:        username,
             connection_type: connection_type,
-            ip: ip,
-            port: port,
-            token: token,
-            is_privileged: is_privileged,
+            ip:              ip,
+            port:            port,
+            token:           token,
+            is_privileged:   is_privileged,
         })
     }
 }
@@ -228,7 +226,7 @@ impl ReadFromPacket for PeerAddressResponse {
     fn read_from_packet(packet: &mut Packet) -> Result<Self, PacketReadError> {
         let username = try!(packet.read_value());
         let ip = try!(packet.read_value());
-        let port = try!(packet.read_port());
+        let port = try!(packet.read_value());
 
         Ok(PeerAddressResponse {
             username: username,