Conversation
| return; | ||
| } | ||
|
|
||
| switch (random.Next(0, 2)) |
There was a problem hiding this comment.
Make this random.NextBool() instead?
There was a problem hiding this comment.
I would keep it because there may be more than two possible payloads in the future
| } | ||
| else | ||
| { | ||
| udpLayer.DestinationPort = 5355; |
There was a problem hiding this comment.
This port was copied from the DnsLayer-Test above because I thought during tests the port is randomly wrong on purpose. I just checked port 5355 and it is used for Link-Local Multicast Name Resolution. So therefor my code with random wrong port doesn't make sense. Was removed by commit 0e40921
| else | ||
| { | ||
| udpLayer.SourcePort = 5355; | ||
| } |
There was a problem hiding this comment.
Lines 432-439 seem to repeat lines 424-431.
| /// The DHCP message structure is shown below: | ||
| /// <pre> | ||
| /// 0 1 2 3 | ||
| /// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 |
There was a problem hiding this comment.
For consistency, it might be better to format this similar to other layers.
| private static class Offset | ||
| { | ||
| public const int Op = 0; | ||
| public const int Htype = 1; |
There was a problem hiding this comment.
Consider not using short names, here and elsewhere.
Op -> Operation
Htype -> HardwareAddressType
Hlen -> HardwareAddressLength
Xid -> TransactionId
Secs -> Seconds
CiAddr -> ClientIpAddress
etc.
| public const int Hops = 3; | ||
| public const int Xid = 4; | ||
| public const int Secs = 8; | ||
| public const int Flags = 10; |
There was a problem hiding this comment.
Instead of having a field for "flags", split it to the different flags (RFC 2131 only has Broadcast).
There was a problem hiding this comment.
Thanks, but I meant split it to different fields:
I think it would make the usage easier.
/// +-------------------------------+------------+------------------+
/// | secs (2) | Broadcase | reserved |
/// +-------------------------------+------------+------------------+
| /// RFC 2131. | ||
| /// Transaction ID, a random number chosen by the client, used by the client and server to associate messages and responses between a client and a server. | ||
| /// </summary> | ||
| public int TransactionId |
There was a problem hiding this comment.
Should probably be uint unless there's a reason to have signed values.
| { | ||
| } | ||
|
|
||
| private void ParseServerHostName() |
There was a problem hiding this comment.
Maybe inline in the getter as it's only used once?
|
|
||
| private void ParseServerHostName() | ||
| { | ||
| if (_serverHostName == null) |
There was a problem hiding this comment.
Prefer if (_serverHostName != null)
return;
There was a problem hiding this comment.
I did it like in DnsDatagram to keep it consistent
| { | ||
| //at the moment we only interpret server host name as sname and ignore options | ||
| byte[] byteServerHostName = ReadBytes(Offset.Sname, 64); | ||
| _serverHostName = Encoding.ASCII.GetString(byteServerHostName).TrimEnd('\0'); |
There was a problem hiding this comment.
Are you sure this is limited to ASCII?
| } | ||
| } | ||
|
|
||
| private void ParseBootFileName() |
There was a problem hiding this comment.
Ditto comments in ParseServerHostName()
| { | ||
| //at the moment we only interpret server host name as sname and ignore options | ||
| byte[] byteServerHostName = ReadBytes(Offset.Sname, 64); | ||
| _serverHostName = Encoding.ASCII.GetString(byteServerHostName).TrimEnd('\0'); |
There was a problem hiding this comment.
Regarding TrimEnd(), what happens if after the null character there's a non null character?
| } | ||
| } | ||
|
|
||
| private void ParseOptions() |
There was a problem hiding this comment.
Ditto some of the comments in the other Parse methods.
| if (_options == null) | ||
| { | ||
| List<DhcpOption> options = new List<DhcpOption>(); | ||
| int pos = IsDhcp ? Offset.OptionsWithMagicCookie : Offset.Options; |
There was a problem hiding this comment.
Use offset instead of pos.
| { | ||
| options.Add(DhcpOption.CreateInstance(this, ref pos)); | ||
| } | ||
| this._options = new ReadOnlyCollection<DhcpOption>(options); |
There was a problem hiding this comment.
Avoid using "this." explicitly.
| /// <summary> | ||
| /// Creates a Layer that represents the datagram to be used with PacketBuilder. | ||
| /// </summary> | ||
| public override ILayer ExtractLayer() |
There was a problem hiding this comment.
Keep methods sorted: public, internal, private.
|
|
||
| /// <summary> | ||
| /// RFC 2131. | ||
| /// Optional server host name |
There was a problem hiding this comment.
End comments with periods.
| public const int Op = 0; | ||
| public const int Htype = 1; | ||
| public const int Hlen = 2; | ||
| public const int Hops = 3; |
There was a problem hiding this comment.
Use relative offsets.
Hops = Hlen + sizeof(byte).
|
|
||
| /// <summary> | ||
| /// RFC 2131. | ||
| /// true if the magic dhcp-cookie is set |
There was a problem hiding this comment.
Replace "true if" with "Whether"
| public const int OptionsWithMagicCookie = 240; | ||
| } | ||
|
|
||
| internal const int DHCP_MAGIC_COOKIE = 0x63825363; |
| { | ||
| get | ||
| { | ||
| if (Length >= Offset.Options + 4) |
There was a problem hiding this comment.
Replace "4" with "sizeof(int)" ?
| /// RFC 2131. | ||
| /// true if the magic dhcp-cookie is set | ||
| /// </summary> | ||
| public bool IsDhcp |
There was a problem hiding this comment.
What is this Datagram if it's false?
|
|
||
| if (options != null) | ||
| { | ||
| length += options.Sum(p => 1 + (!(p is DhcpPadOption || p is DhcpEndOption) ? 1 : 0) + p.Length); //Type + Len? + Option |
There was a problem hiding this comment.
Replace "p" with "option".
Replace "1" with "sizeof(byte)".
Can you rely on p.Op instead of its class type?
There was a problem hiding this comment.
Changed in c1550dd.
Yes, option.OptionCode makes much more sense.
|
|
||
| if (options != null) | ||
| { | ||
| length += options.Sum(p => 1 + (!(p is DhcpPadOption || p is DhcpEndOption) ? 1 : 0) + p.Length); //Type + Len? + Option |
There was a problem hiding this comment.
Replace "//Type" with "// Type".
|
|
||
| internal static void Write(byte[] buffer, int offset, DhcpMessageType messageType, ArpHardwareType hardwareType, byte hardwareAddressLength, byte hops, int transactionId, ushort secondsElapsed, DhcpFlags flags, IpV4Address clientIpAddress, IpV4Address yourClientIpAddress, IpV4Address nextServerIpAddress, IpV4Address relayAgentIpAddress, DataSegment clientHardwareAddress, string serverHostName, string bootFileName, bool isDhcp, IList<DhcpOption> options) | ||
| { | ||
| buffer.Write(ref offset, (byte)messageType); |
There was a problem hiding this comment.
Be consistent with the name - either "op" or "messageType".
| internal static void Write(byte[] buffer, int offset, DhcpMessageType messageType, ArpHardwareType hardwareType, byte hardwareAddressLength, byte hops, int transactionId, ushort secondsElapsed, DhcpFlags flags, IpV4Address clientIpAddress, IpV4Address yourClientIpAddress, IpV4Address nextServerIpAddress, IpV4Address relayAgentIpAddress, DataSegment clientHardwareAddress, string serverHostName, string bootFileName, bool isDhcp, IList<DhcpOption> options) | ||
| { | ||
| buffer.Write(ref offset, (byte)messageType); | ||
| buffer.Write(ref offset, (byte)hardwareType); |
There was a problem hiding this comment.
Consider relying on the defined offsets instead of using ref.
| offset += Offset.Sname - Offset.ChAddr; | ||
| if (serverHostName != null) | ||
| { | ||
| buffer.Write(offset, new DataSegment(Encoding.ASCII.GetBytes(serverHostName))); |
There was a problem hiding this comment.
Consider also writing the null termination.
| /// <summary> | ||
| /// Hardware address length | ||
| /// </summary> | ||
| public byte HardwareAddressLength { get; set; } |
There was a problem hiding this comment.
Shouldn't the hard address length just be derived from the hardware address?
There was a problem hiding this comment.
I would separate it from the ClientHardwareAddress to allow setting all ClientHardwareAddress-Bytes.
ex. HardwareAddressLength = 6 but the user still wants to set byte 7 - 16
Maybe automatically set the HardwareAddressLength to 6 if the ClientMacAddress is set. What do you think about that?
There was a problem hiding this comment.
I see, makes sense.
I didn't realize the Hardware Address field has constant size, regardless of the Hardware Address Length field.
It makes sense to set it to 6 (you can use MacAddress.SizeOf instead of 6) when the helping property ClientMacAddress is used, just make sure it's clear from the property comment.
| /// <param name="nextLayer">The layer that comes after this layer. null if this is the last layer.</param> | ||
| public override void Finalize(byte[] buffer, int offset, int payloadLength, ILayer nextLayer) | ||
| { | ||
| } |
There was a problem hiding this comment.
I don't think you need to override this if you don't do anything.
| namespace PcapDotNet.Packets.Dhcp | ||
| { | ||
| /// <summary> | ||
| /// RFCs 2131. |
There was a problem hiding this comment.
Replace "RFCs" with "RFC".
| /// <summary> | ||
| /// RFCs 2131. | ||
| /// </summary> | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1008:EnumsShouldHaveZeroValue")] |
There was a problem hiding this comment.
Consider adding a None value instead of suppressing this.
| /// create new DhcpBootfileNameOption | ||
| /// </summary> | ||
| /// <param name="bootfileName">Bootfilename</param> | ||
| public DhcpBootfileNameOption(string bootfileName) : base(bootfileName, DhcpOptionCode.BootfileName) |
There was a problem hiding this comment.
Maybe rename bootfilename to bootFilename ?
There was a problem hiding this comment.
I always tried to keep the names according the RFC specification. Only if it was not possible (spaces, conflict with existing names,...) I changed the name
There was a problem hiding this comment.
Well, in general in Pcap.Net, it's attempted to have the names readable so they can be used without being familiar with the RFC.
In this case, if "bootfile" is considered a word, this makes sense. Otherwise it makes more sense to have bootFilename.
| } | ||
|
|
||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1502:AvoidExcessiveComplexity")] | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1506:AvoidExcessiveClassCoupling")] |
There was a problem hiding this comment.
Consider solving these maintainability issues by using registration mechanism for the options, similar to what is being done in other protocols.
bricknerb
left a comment
bricknerb
left a comment
There was a problem hiding this comment.
Wow thanks!
Awesome work!
I didn't go over all the details.
Specifically, I didn't check code matches the RFC and I assume it's ok.
One missing part that can help make sure the code indeed matches the RFC is the Wireshark comparison code. Let me know if you need help with this.
I've written a bunch of comments, many of them should be applied on many places in the code.
Let me know if you can address these comments. Otherwise, I'll have to see if I can take care of them myself.
Hopefully I didn't miss any.
sizeof instead of constant value
(also some changes for "End comments with periods.")
Consider relying on the defined offsets instead of using ref.
…s (RFC 2131 only has Broadcast).
bricknerb
left a comment
bricknerb
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
Let me know when you want me to make a full pass on the code.
| { | ||
| } | ||
|
|
||
| [DhcpOptionReadRegistration(DhcpOptionCode.ArpCacheTimeout)] |
There was a problem hiding this comment.
Any reason not to have this attribute on the classes instead of the creation method?
As long as you don't have two creation methods for the same class, I think putting this on the class is easier to maintain.
2 participants
Added support for DHCP (RFC 2131) as requested in #55.
Also includes tests.