On Sat, 17 Jul 2010 18:07:56 +0100 Matthew Toseland wrote: > 866a2aa9ee74434d6ad4328dc8c5edaa75b779f6 > - synchronize when allocating a sequence number!! Fixed in fee766aeba5c167497031357af68d5b446ffa090
> 59e7088ca13761253eb1729b9e4c9f73af6ae205 > - Here and elsewhere sign extension on bytes can trip you up. You > need to do & 0xff on the byte values, or use one of our provided > methods for decoding, or mask the whole thing afterwards e.g. value = > value | 0xffffff. AFAIK all of these are fixed in various later commits > 78aced73420599f544cf6aefca188ca9963b28d4 > - assertTrue(s.contains(3, 2)); > - this should throw! Copy paste fail... Test fixed in b49ad5b3cc9f3b5d6b4349f64164ab1acc59c6b1 Throws in 67d01ba9446c29facfc40cfe3307a9f3b3f7d1b0 > f32ee7587ce8a3fa80574b35505aa66a080ef084 > + //TODO: If the other side resends a > packet after we have gotten all the data, we will > + //never ack the resent packet > - The standard solution to this is to ack resent packets. If we have > processed a packet and we get a resend for it, ack it. This specific issue doesn't exist anymore. The larger problem will be solved when we find a good way to deal with the message ids > cd7d374b7823906a46a46a417fdface43bbe188a > - I'm not sure this is wise - if we use a full HMAC on big packets, > that might kick the encrypted sequence number into the second block, > thus becoming unpredictable? OTOH if we use an HMAC of the encrypted > data (keyed from a key other than the encryption key), and don't > encrypt it itself, it won't be a problem. Shouldn't be a problem since the encryption is done starting at the sequence number. (using different keys for lots of things is on my todo-list) > 0a92c3f7a14d1f44f1f25013d045fbbeb2ff867b > - Can isFragmented() be bogus here with border cases? It is > calculated based on the maximum header length - if it returns false > and then we discover that with the actual header length we don't need > to fragment, what happens? That code has been changed a lot since that commit, but I'm pretty sure the fragmentation works correctly atm. Fragmentation logic was rewritten in e498d8d502a64e1413ebdf69e05ce0940d140d02. > 17096df5098e53881db1a6957c41b6c83e0ad4ec > - If frag == null, do we lose the message from the queue here? Yes. Fixed in 1cc94e41b6e7b64ceaf249426d193c843b6baf82 > 5c537e76f15e8ef9e4ed974e5a41a31fd720f3b1 > - Doesn't item itself have a method to call all the acks? it should... It doesn't, and what PacketTracker did looked non-trivial, so I've added it to my todo-list. In case anyone would like to fix it, the duplicated code is at: src/freenet/node/MessageWrapper.java:37 src/freenet/node/PacketTracker.java:542 src/freenet/node/PacketTracker.java:588 (commit 1cc94e41b6e7b64ceaf249426d193c843b6baf82) > bc1eb2fa33c0b2063992df3fb9b207c8c7b66f0c > - The unit test doesn't test what it says. The bytes should each be > different to be sure about this. Fixed in 3bc052c6c3f16e8beedf152d0b0088bb6e3109a0 > 7a316c720fcaed368d7ad35f8a1262aa4f91dce7 > - it shouldn't be added if it's bad Fixed in 24cd2e093de160ebdb8de583cdb1727420e3bee0 > b58aa213ed98f814eaab2c9893d1b12d4578c04e > - remove the sentPacket when returning due to no key??? Fixed in 70b1586ead668692836fc58715045945843cf002 > a1acd68b51cc124a11ed1d496c82f82ce279da3a > - Don't count the RTT slots that haven't been filled in! Fixed in 1ceb102eeef5009ee43161ad5562e4bef790d022 > 8f561a3ce585170c55a678e0cc0d62b6ab639a25 > - Use entrySet() and Map.Entry<>. Fixed in 394e5151be5422452a939d4d76444069644ffba2 > 899528d44d43b42d2ca8a304d9ecfe4847a5c50b > Use maximum not average of last 10 RTTs > - Is this a good idea? Probably not. IIRC I didn't mean to push that, so reverted as 1da1bcfcb38f220ce21e1f3d9ef6fedb73fe24db -- Martin Nyhus -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 198 bytes Desc: not available URL: <https://emu.freenetproject.org/pipermail/devl/attachments/20100727/05969033/attachment.pgp>
