bep47 #526
bep47 #526
Conversation
|
I didn't review it in full - i'll do that tomorrow! |
Fails because md5sum isn't present in the torrent
as the previous file in the torrent. Ensures empty files at the end of a torrent are never the only file in a piece. Makes some tests green again.
| @@ -116,6 +116,7 @@ public void LoadV2OnlyTorrent () | |||
| Assert.AreEqual (InfoHash.FromHex ("caf1e1c30e81cb361b9ee167c4aa64228a7fa4fa9f6105232b28ad099f3a302e"), V2OnlyTorrent.InfoHashes.V2); | |||
| } | |||
|
|
|||
| // TODO: fails with MonoTorrent.TorrentException : the 'piece layers' dictionary did not contain an entry for the file 'tbl-tint.mpg' | |||
There was a problem hiding this comment.
I pushed a tiny change which fixes this. When zeroing out the padding for the last file in the torrent you had forgotten to copy over the RootHash property. This is what caused the file to not be found in the piece layers dictionary :)
|
Overall this looks pretty good! The extra validations are great. I've had a busy week or so in work and am flying out for a 2 week holiday on Saturday. As such I'm not likely to get back to you with reviews until I'm back :( I haven't had much time to thoroughly think through Thanks for the work so far, and i'm looking forward to getting this merged after i'm back! |
|
|
||
| // padding of last torrent must be 0. | ||
| var last = files.Last (); | ||
| files[files.Count - 1] = new TorrentFile (last.Path, last.Length, last.StartPieceIndex, last.EndPieceIndex, last.OffsetInTorrent, last.PiecesRoot, TorrentFileAttributes.None, 0); |
There was a problem hiding this comment.
I added last.PiecesRoot here to propagate that data!
|
Alan no worries on the review, have a great holiday! |
|
I verified that md5 hashing is also broken on master, |
This will unblock bep 0047 from merging as md5 hashes are computed correctly now. The sha sums are the same before and after now!
|
@jpmikkers I fixed the MD5 issue last night - finally! What's left to complete in this change? If it's pretty much done now I'll give it a final review and land any minor tweaks as needed before, or after, merging :) |
|
I also had a small fix for the remaining // TODO: JMIK (the endindex rounding issue) but suddenly vs2022 doesn't let me push that commit, groan Torrent.cs line 790 should probably be this, can you confirm?: |
Whoops :) Tests are always important!
You were right! |
| @@ -232,6 +234,11 @@ internal async Task<BEncodedDictionary> CreateAsync (string name, List<InputFile | |||
| info["name"] = (BEncodedString) name; | |||
| AddCommonStuff (torrent); | |||
|
|
|||
| foreach (var file in files.Take (files.Count - 1)) { | |||
| file.Padding = (UsePadding && file.Length > 0) ? PieceLength - (file.Length % PieceLength) : 0; | |||
There was a problem hiding this comment.
I just realised that this would add PieceLength padding if file.Length is an exact multiple of PieceLength.
I added a test and fixed the bug!
| startOffsetInTorrent = i > 0 ? results[i - 1].OffsetInTorrent : 0; | ||
| // catch pathological case of too much padding | ||
| if (pieceEnd != pieceEndWithPadding) { | ||
| throw new ArgumentException ("A file in the torrent has more padding than needed."); |
There was a problem hiding this comment.
This check was helpful as it triggered when I created the testcase to see what happened when file.Length == PieceLength :)
|
I gave this a few rounds of checks and didn't find any more issues! Thanks for the great work! I'll let this continue baking as I (slowly) slipstream in the final bits for bep52 :D Thanks for the patch! |
Todo:
not sure what to do on torrent.cs line 429 for hybrid torrents..