On Mon, 2009-02-09 at 16:11 -0500, Chuck Lever wrote:
> Hi Ian-
>
> On Feb 8, 2009, at 6:55 AM, Ian Dall wrote:
> > On Wed, 2009-02-04 at 13:47 -0500, Trond Myklebust wrote:
> >> On Wed, 2009-02-04 at 13:44 -0500, Chuck Lever wrote:
> >>> Right. So, then, can nfs_match_client() use the same address
> >>> comparison method as nfs_find_client()? I'm inclined to think
> >>> nfs_match_client() can use a more straightforward address comparison
> >>> method.
> > Log:
> > ======================
> > sockaddr structures can't be reliably compared using memcmp()
> > because there are padding bytes in the structure which can't be
> > guaranteed to be the same even when the sockaddr structures refer to
> > the same socket.
>
> It is also the case that IPv6 socket address structures contain other
> fields that don't have to match, like sin6_scope_id. I think that is
> worth noting in the patch description.
>
> I don't see anything immediately wrong with this approach, but I made
> a couple of additional minor comments below.
>
> > Signed-off-by: Ian Dall <ian@xxxxxxxxxxxxxxxxxxxxx>
Apparently this should be "From" (for the author) not "Signed-off-by"
According to Andrew Morton commenting on a completely different patch.
> > ======================
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 9b728f3..04b11d9 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -272,6 +272,42 @@ static int nfs_sockaddr_match_ipaddr(const
> > struct sockaddr *sa1,
> > }
> > #endif
> >
> > +/* Test if two socket addresses are the same family, ipaddr and
> > port */
>
> Instead of repeating what the function does, can this comment explain
> why the NFS client needs to match on the address and port, for example?
>
> Explaining why it is different than the existing
> nfs_sockaddr_match_ipaddr() function would be helpful for future
> generations.
Wouldn't this be more appropriate where the function is called? I know
that it is only called from one place, but still, I would have thought
the obvious thing to do would be document what a function does where you
define it and document why you are calling it when you are calling it.
Either way, I don't actually *know* why we need to match on port number!
I have to confess no deep understanding of the difference between
nfs_match_client and nfs_find_client. I only did a bisection to find the
commit which caused the regression, then fixed it.
If someone who does understand wants to contribute a description, that's
fine by me! Following the principle of one change per patch, it may as
well be a different commit since it would relevant regardless of my
patch. The current code (with the memcmp) compares everything, it's just
that it compares too much.
> As a stylistic point, these days we prefer to use a helper function
> instead of curly braces inside the arms of the switch. It will also
> remove the need to fold those lines.
>
> I have used this style in the past, but there is general consensus now
> that helper functions are easier to read, and are therefore preferred.
Yes, I blame it on the prevalence of OO programming where it is routine
to have a helper functions which to do nothing more than get a field out
of a structure ;-(
Here is a new patch with helper functions, improved ipv6 comparison
(though I can not test this and my understanding of ipv6 is almost
certainly incomplete), better comments and a better log entry.
Log:
======================
sockaddr structures can't be reliably compared using memcmp()
because there are padding bytes in the structure which can't be
guaranteed to be the same even when the sockaddr structures refer to
the same socket. Instead compare all the relevant fields. These are
sin_addr and sin_port number for IPv4. In the case of IPv6 they are
sin6_addr, sin6_port and sin6_scope_id, which is relevant for link
local addresses. The sin6_flowinfo field only affects QoS and is not
compared.
From: Ian Dall <ian@xxxxxxxxxxxxxxxxxxxxx>
======================
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..efdae09 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -272,6 +272,62 @@ static int nfs_sockaddr_match_ipaddr(const struct sockaddr *sa1,
}
#endif
+/* Test if two ip4 socket addresses have the same ipaddr and port number */
+static int nfs_sockaddr_cmp_ip4 (const struct sockaddr_in * saddr1,
+ const struct sockaddr_in * saddr2)
+{
+ /* Compare ip4 address */
+ if (likely(saddr1->sin_addr.s_addr !=
+ saddr2->sin_addr.s_addr)) {
+ return 0;
+ }
+ /* Compare port */
+ return (saddr1->sin_port == saddr2->sin_port);
+}
+
+/* Test if two ip6 socket addresses have the same ipaddr, scope_id and
+ * port number.
+ */
+static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
+ const struct sockaddr_in6 * saddr2)
+{
+ /* Compare ip6 address */
+ if (likely(!ipv6_addr_equal(&saddr1->sin6_addr,
+ &saddr1->sin6_addr))) {
+ return 0;
+ }
+
+ /* Compare scope_id. This may only matter for link local
+ * addresses, but it is cheap, and should be harmless, to just
+ * compare it anyway.
+ */
+ if (saddr1->sin6_scope_id != saddr2->sin6_scope_id)
+ return 0;
+ /* Don't compare sin6_flowinfo since it is to do with QoS, but
+ * doesn't effect the packets source or destination.
+ */
+ /* Compare ports */
+ return (saddr1->sin6_port == saddr2->sin6_port);
+}
+
+/* Test if two socket addresses represent the same actual socket */
+static int nfs_sockaddr_cmp(const struct sockaddr *sa1,
+ const struct sockaddr *sa2)
+{
+ if (sa1->sa_family != sa2->sa_family)
+ return 0;
+
+ switch (sa1->sa_family) {
+ case AF_INET:
+ return nfs_sockaddr_cmp_ip4 ((const struct sockaddr_in *) sa1,
+ (const struct sockaddr_in *) sa2);
+ case AF_INET6:
+ return nfs_sockaddr_cmp_ip6 ((const struct sockaddr_in6 *) sa1,
+ (const struct sockaddr_in6 *) sa2);
+ }
+ return 0;
+}
+
/*
* Find a client by IP address and protocol version
* - returns NULL if no such client
@@ -344,8 +400,10 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data)
{
struct nfs_client *clp;
+ const struct sockaddr *sap = data->addr;
list_for_each_entry(clp, &nfs_client_list, cl_share_link) {
+ const struct sockaddr *clap = (struct sockaddr *)&clp->cl_addr;
/* Don't match clients that failed to initialise properly */
if (clp->cl_cons_state < 0)
continue;
@@ -358,7 +416,7 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
continue;
/* Match the full socket address */
- if (memcmp(&clp->cl_addr, data->addr, sizeof(clp->cl_addr)) != 0)
+ if (!nfs_sockaddr_cmp(sap, clap))
continue;
atomic_inc(&clp->cl_count);
--
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
