On Wed, 2009-02-04 at 12:43 -0500, Chuck Lever wrote: > Hi Ian- > > On Feb 4, 2009, at Feb 4, 2009, 7:52 AM, Ian Dall wrote: > > On Tue, 2009-02-03 at 17:16 -0500, Chuck Lever wrote: > >> On Feb 3, 2009, at Feb 3, 2009, 4:56 PM, Ian Dall wrote: > >>> OK here is version 2 of the patch. It is half the size of version 1, > >>> although the resultant client.c is slightly bigger and IMO slightly > >>> inferior than for version 1, but certainly it is around the margin. > >> > >> Why do you think this is inferior? This way we are checking exactly > >> the fields in each sockaddr that need to be checked, and using the > >> same equality test that most other parts of the NFS client use. > > > > There is duplicated code, namely the function headers for > > nfs_sockaddr_match_ipaddr() and nfs_sockaddr_match_port(). > > > > Granted this doesn't add to the executable size in any way because of > > the conditional compilation, but it is still a potential source of > > trouble down the track when one function definition gets changed and > > another doesn't and further more, it won't show up unless you test > > both > > IPV6 and non IPV6 configurations. Having function headers in two > > places > > makes following the code with etags more confusing. And even without > > etags you have to scroll up and down to see the two versions of the > > same > > function. > > > > Also nfs_sockaddr_match_ipaddr4() is trivial, taking 7 lines of code > > (counting the call and the function definition) to do what can be > > expressed in 2 and has additional overhead, albeit small. > > > > None of these are a big deal, which is way I said *slightly* inferior. > > > > I'm not exactly sure what you didn't like about my first patch. > > Maybe it > > was just too big (fair enough, more to go wrong), or maybe you didn't > > like the test return; test return; ... style, or maybe you didn't > > like > > the use of a port_match flag. > > I think you are trying to address two separate issues here in a single > patch. Replacing the memcmp() should be done in one patch, and any > code clean up should be done in a second. Understood. It arose because I it affected the code I was introducing. My original patch (on bugzilla, not this list) introduced a single function to compare ports. Then after the conditional compilation was added, I would have had to add two versions of the same function, which seemed bad practice. -- Ian Dall <ian@xxxxxxxxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
