Hi Peter,

I’ll try to sumarize it, so we’ll see what needs (should) to be changed. I’ve checked the {Torrent, TrackerResponse}Parser classes and I think that there is currently no problem with Torrent metainfo parsing and validation, so we don’t need an extra validation class for it and we should focus only on the TrackerResponseParser class - do you agree?

In the TrackerResponseParser class, the only mandatory fields are ‘interval’ and ‘peers’. ‘interval’ is easy to check because it simply must be a greater or equal to zero number, so there is no problem with it from my point either. I agree that it’s proper to change the ‘peers’ part. And now “how” to do it.

Your code snippet seems fine to me. So, if I understand it correctly, let me describe the ‘peers’ parsing process one again, taken your code snippet in mind:

• ‘ip’ - just get a raw string and do nothing special with it
• ‘port’ - get a number, check the length and if it’s greater than quint16::max() or negative, then set it to 0, otherwise cast it to quint16 -
• ‘peer_id’ - get a raw string, check it’s length and if it’s not valid, then pass an empty string instead of it

Then after we have got the tracker response, we could iterate over the list or peer URIs and try to connect to each using a helper object (e.g. transferSessionFactory.createSession (peerUri, infoHash);). If the factory responds with a invalidUri (uri), the uri is dropped, if it responds with connectionError (uri) the uri could be stored to try again later (check with BT specs), or it could respond with sessionEstablished (uri).

That’s just the rough idea. I think having the factory actually validate the peer URIs is flexible in a way that we would even add support for DNS names the ip field only by touching the session factory or without touching it at all (e.g. session factory uses a transport factory which takes care of DNS lookups).

If the described parsing process I’ve done is correct, than I agree! This way we won’t need to check ‘ip’ and everything will be simplified. I’d maybe add some comments to the class (TrackerResponse) descritpion (or to getters?) what can a user expect when calling some getter function (something like: “Peer id can be either a string that’s 20B long (that means that it was parsed correctly), or an empty string (original peer id was incorrect).”, etc.).

Let me know what you think about what I’ve written. Maybe I can then refactor Tracker{Request, Response} to use Uri instead of the current PeerInfo, remove the PeerInfo class (I think we won’t needed it, will we?), update TrackerResponseParser (’peers’ parsing and loading) and other related classes (like TrackerRequestSession).

Regards,

Petr