Re: [Packetnet-devel] New Patches

[Chris M. <chm...@gm...>]

On Sun, Mar 14, 2010 at 12:05 AM, Evan Plaice <eva...@gm...> wrote:
> Commit 1: Fix Checksum updates
>
> TCPPacket.cs
> * Remove the logic fork from CalculateChecksum
> * Add UpdateChecksum to handle internal updates
>
> Commit 2: Cleanup IP Checksum Operations
>
> IpPacket.cs
> * Remove ValidIPChecksum abstract
>   * IP checksums only apply to IPv4
> * Remove UpdateIPChecksum abstract
>   * IP checksums only apply to IPv4
> * Remove ComputeIPChecksum abstract
>   * IP checksums only apply to IPv4
>
> IPv6Packet.cs
> * Remove ValidIPChecksum
>   * IPv6 does not implement checksums
> *Remove UpdateIPChecksum
>   * IPv6 does not implement checksums
> * Remove ComputeIPChecksum
>   * IPv6 does not implement checksums
>
> IPv4Packet.cs
> * ValidIPChecksum override removed
>   * Abstract no longer exists in IpPacket.cs
> * ComputeIPChecksum override removed
>   * Abstract no longer exists in IpPacket.cs
> * UpdateIPChecksum override removed
>   * Abstract no longer exists in IpPacket.cs
>
> TcpPacket.cs
> * Fixed ValidChecksum property
>   * Only checks the IPChecksum of IPv4 packets now
>
> Commit 3: Join TcpPacket.cs to TransportPacket.cs
>
> TcpPacket.cs
> Make a subclass of TransportPacket.cs
>
> Commit 4: Move Transport Checksumming to TransportPacket.cs
>
> TransportPacket.cs
> * Added preprocessor directive to enable logging without crashing the build
> * Moved IsValidTransportLayerChecksum from IPPacket.cs
> * Moved ComputeTransportLayerChecksum from IPPacket.cs
> * Added using MiscUtil.Conversion
>   * Need for EndianBitConverter
> * Renamed IsValidTransportLayerChecksum to IsValidChecksum
>   * Renamed for simplicity
> * Renamed ComputeTransportLayerChecksum to ComputeChecksum
>   * Renamed for simplicity
>
> IpPacket.cs
> * Changed access level of AttachPseudoHeader to public
> * Removed IsValidTransportLayerChecksum
>   * Method is Transport Layer specific, therefore it should be in
> TransportPacket.cs
> * Removed ComputeTransportLayerChecksum
>   * Method is Transport Layer specific, therefore it should be in
> TransportPacket.cs
>
> IPv4Packet.cs
> * Changed access level of AttachPseudoHeader to public
>
> IPv6Packet.cs
> * Changed access level of AttachPseudoHeader to public
>
> Commit 5: Change all Compute[x]() to Calculate[x]()
>
> IPv4Packet.cs
> * Renamed ComputeIPChecksum to CalculateIPChecksum
>   * Renamed to match common naming convention
> * Updated comments to reflect changes
> * Updated method calls to reflect changes
>
> TransportPacket.cs
> * Renamed ComputeChecksum to CalculateChecksum
>   * Renamed to match common naming convention
> * Updated comments to reflect changes
> * Updated method calls to reflect changes
>
> TcpPacket.cs
> * Updated method calls to reflect changes
> IpPacket.cs
> * Updated Comments to reflect changes
>
> ------------------------------------------------------------------------------
> Download Intel&#174; Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> _______________________________________________
> Packetnet-devel mailing list
> Pac...@li...
> https://lists.sourceforge.net/lists/listinfo/packetnet-devel
>
>

The subject name of the first patch isn't very descriptive. Plus you
are renaming methods in the same patch that you are changing the
logic. You've really got to stop mixing the two but patch 0001 will go
in.

Patch 0002 looks like a good reduction in total lines of code.

The subject for patch 0003 isn't very accurate. I'll fix it before
applying it since "join" isn't really a commonly used term when
describing inheritance.

Why are you exposing the AttachPseudoIPHeader() methods as public? If
those aren't intended to be called by the end user then its best to
keep them hidden by marking them as something other than public.

Patch 0005 looks good.


So 0004 is going to be holding up patch 0005. I'll apply the rest
right now. The Packet.Net api has to be carefully controlled. Users
should be presented with the minimal number of properties, methods and
constructors that they need and I didn't see the justification for
exposing the pseudo header methods to them.

Chris