Hi David,
Thanks for the patch! It’s quite neat :-)! I just applied it on your Kad branch:
http://bzr.calitko.org/developers/david/calitko-Kad
and the latest revision there is 81.
Here are my answers to your notes/questions in the patch:
+// Why do I need to declare namespaces here, but not in, for example,
+// CallbackReq.cpp?
Because in the cpp files you should include Imports.cpp and not Imports.h, which should be included in headers only. The difference is that Imports.h contains mostly forward declaration, whereas Imports.cpp contains the complete declarations of all classes types from other packages. That is to decrease the header file dependencies. The cpp futher imports all names from the current namespace.
+ Utilization: This packet is used when a client wants to join the Kad net-
+ work. This is the first packet sent. This packet must be sent to a known
There is no need to break words like net-work. Doxygen will output “net- work”. If the word exceeds the 80 char border then break the line before the word and not in the middle.
+ Format:
+ PEER (sender) [25]
+
+ \note should I include some copyright about this format format? It belongs
+ to eMule’s Opcodes.h
Well, is it just the two lines? We can use our own format that is also more expressive, like:
Payload:
0..15 id Kad ID of the peer.
16..19 ip; Sender or Receiver IP address.
20..21 udpPort Incoming UDP Port used in Kad network.
22..23 tcpPort Incoming TCP Port used in eDonkey network.
24 notUsed Reserved for future use.
But that info better be added to the docs of PeerInfoPacket and then only add a reference to it from the template instantiations:
Payload:
see PeerInfoPacket
What do you think of this?
+#ifndef __PROTOCOLS__KAD__PACKETS__PEER__INFO__PACKET
+#define __PROTOCOLS__KAD__PACKETS__PEER__INFO__PACKET
Please use _ for word separator or . and __ for directory separator. I’ve fixed this and similar ones in your patch.
I guess you’ll need getters for PeerInfoPacket and a ctor that takes arguments to initialize the payload fields. It’ll become obvious you have to add them when you write the unit tests.
+ UInt128 id; //!< Kad ID.
...
Why don’t you take QUuid instead ? It’s a 128bit UUID struct with a bunch of useful functions. BinaryReader and BinaryWriter can be get the new QUuid readUuid(); and void writeUuid (const QUuid &);.
Please also protect Imports.h with #ifndef. That have slipped last time. It’s not too bad but it’s better.
+ // Utils namespace conflicts with Protocols::Kad::Utils !!!! :S
You’ve resolved that well and it’s great you’ve written a comment about the issues.
+
+ \todo Find the way to support both protocol types (compressed and plain).
I might have an idea. Let’s implement only uncompressed packets. We’ll have anyway a PacketFactory that will read and possibly write packets, so the factory can take care of compressing/uncompressing the rawData and modifying the header field. Can it be that a specific bit in the packet type is flipped to indicate compressed payload?
Best regards,
Peter
David Alfonso wrote:Hi all, I’ve been working on another kind of packets which contains the information of a peer. I’ve also corrected some errors from my last revision.
To-Do:
- Make tests.
- Keep on studying the Kad protocol and packets.
- Implement more packets.
- Support packet compression.
There’s a diff attached to this post. You should apply it over the last revision of the Kad branch (http://bzr.calitko.org/developers/david/calitko-Kad)
best regards,
David