Rust Torrent Parser

Question

I wrote a simple Torrent file parser. The following program accepts a file and builds a Torrent structure based on the contents on the file. If the file is invalid, the app crashes (as intended).

It's my first time writing anything >50 lines in rust, so I'm looking for improvements :)

---

use std::collections::BTreeMap;

use bencode::{Bencode, Encoder};
use bencode::util::ByteString;

#[derive(Default, Debug)]
pub struct Torrent {
    announce: String,
    announce_list: Vec<String>,
    name: String,
    comment: String,
    multi_file: bool,
    piece_length: i32,
    length: i64,
    creation_date: String,
    total_size: i32,
}


impl Torrent {
    pub fn new() -> Self {
        return Torrent {
            ..Default::default()
        };
    }
    pub fn populate_from_bencode(&mut self, b: Bencode) -> Bencode {
        if let Bencode::Dict(dict) = b {
            dict.into_iter().for_each(|(s, b)| {
                if s.as_slice() == b"announce" {
                    self.announce = extract_string(b).unwrap_or_else(|| panic!("unable to extract announce"));
                } else if s.as_slice() == b"name" {
                    self.name = extract_string(b).unwrap_or_else(|| panic!("unable to extract name"));
                } else if s.as_slice() == b"pieces" {} else if s.as_slice() == b"comment" {
                    self.comment = extract_string(b).unwrap_or_else(|| panic!("unable to extract comment"));
                } else if s.as_slice() == b"creation-date" {
                    self.creation_date = extract_string(b).unwrap_or_else(|| panic!("unable to extract creation-date"));
                } else if s.as_slice() == b"info" {
                    self.populate_from_bencode(b);
                } else if s.as_slice() == b"length" {
                    if let Bencode::Number(length) = self.populate_from_bencode(b) {
                        self.length = length;
                    }
                } else if s.as_slice() == b"announce-list" {
                    if let Bencode::List(x) = self.populate_from_bencode(b) {
                        self.announce_list = x.into_iter()
                            .map(|x| extract_list(x).unwrap_or_else(|| panic!("list")))
                            .flatten()
                            .map(|x| extract_string(x).unwrap_or_else(|| panic!("unable to extract string")))
                            .collect();
                    }
                }
            });

            return Bencode::Empty;
        } else {
            return b;
        }
    }
}

fn extract_list(b: Bencode) -> Option<Vec<Bencode>> {
    if let Bencode::List(s) = b {
        return Some(s);
    }
    return None;
}

fn extract_pieces(b: Bencode) -> Option<Vec<u8>> {
    if let Bencode::ByteString(x) = b {
        return Some(x);
    }
    return None;
}


fn extract_string(b: Bencode) -> Option<String> {
    if let Bencode::ByteString(s) = b {
        return Some(String::from_utf8_lossy(&s).to_string());
    }
    return None;
}


//decoder.rs

use std::fs::File;
use std::io::Read;

use bencode::Bencode;

use crate::torrent;
use crate::torrent::Torrent;


pub fn decode_file_into_torrent(path: &'static str) -> Result<Torrent, Box<dyn std::error::Error>> {
    let mut file = File::open(path)?;
    let v = file.bytes().map(|x| x.unwrap()).collect::<Vec<u8>>();
    let bencode = bencode::from_vec(v).unwrap();
    let mut t = Torrent::new();
    t.populate_from_bencode(bencode);
    Ok(t)
}







#[cfg(test)]
mod tests {
    use crate::decoder;

    use super::*;

    #[test]
    fn test_parser() {
        let t = decoder::decode_file_into_torrent("src/test.torrent");
        assert!(t.is_ok());
    }
}

EDIT: Fighting with the compiler and debugging this been an absolute nightmare :(

Answer 1

For deserialization, its a lost easier if you use serde. Then you can do something like this:

use serde_derive::Deserialize;

// the Deserialize derive is provided by serde, it causes the code
// to be generated that is needed for deserialization
#[derive(Default, Debug, Deserialize)]
// this tells serde you want to use kebab-case, which is 
// with dashes instead of underscores for field names
#[serde(rename_all = "kebab-case")]
// this tells serde to default values to the empty case if not provided
#[serde(default)]
pub struct Torrent {
    announce: String,
    announce_list: Vec<Vec<String>>,
    name: String,
    comment: String,
    multi_file: bool,
    piece_length: i32,
    length: i64,
    creation_date: String,
    total_size: i32,
}

Then you can read a torrent into this struct with:

let torrent :Torrent = serde_bencode::from_bytes(&v).unwrap();

But let's ignore that for a moment, and look through your code:

impl Torrent {
    pub fn new() -> Self {
        return Torrent {
            ..Default::default()
        };
    }

You don't need the explicit return statement. If you leave the semicolon off of the last statement in a function, the expression of that statement will be returned. Further, you don't use to unpack the Default::default() into the Torrent. Default::default() will already return a torrent. So this function could be written

pub fn new() -> Self {
    Default::default()
}

Moving on

pub fn populate_from_bencode(&mut self, b: Bencode) -> Bencode {

It is unusual that you would modify an object based on the input. Instead, you typically return a new object based on the bencode input. It also odd that you would return the Bencode. You ignore this output in any case, so its unclear why you are doing this.

    if let Bencode::Dict(dict) = b {
        dict.into_iter().for_each(|(s, b)| {

I would do for (s,b) in dict { instead. In my mind, it is easier to follow standard for loops rather than doing a for_each.

            if s.as_slice() == b"announce" {

Rather then having a repeated if statement like this, use match

                self.announce = extract_string(b).unwrap_or_else(|| panic!("unable to extract announce"));

Option has an expect method, which takes a string and panics if the option is none. However, if you are going to panic anyways, why not panic inside extract_string?

            } else if s.as_slice() == b"length" {
                if let Bencode::Number(length) = self.populate_from_bencode(b) {
                    self.length = length;
                }

In this case, you ignore a non-numeric length. This suspicous, I'd almost certainly rather get an error then silently failing to load length.

pub fn decode_file_into_torrent(path: &'static str) -> Result<Torrent, Box<dyn std::error::Error>> {

Why are you taking a &'static str. It would seem that this function doesn't have take a str that lasts for the whole lifetime of your application. Further, I prefer to take the std::path::Path and friends types for parameters that are actually paths.

    let mut file = File::open(path)?;

Why, in this one case, are you returning an error instead of panicking?

    let v = file.bytes().map(|x| x.unwrap()).collect::<Vec<u8>>();

Firstly, you can do the following:

    let v = file.bytes().collect::<Result<Vec<u8>, _>>();

Rust knows how to construct a result of vec from iterator over results.

Secondly, there is a specific method for reading the rest of the file

    let mut v = Vec::new();
    file.read_to_end(&mut v).unwrap();

Third, this a function that reads the whole file into bytes from just the path

    let v = std::fs::read(path)?;