On Feb 15, 2009, at 10:32 AM, Ian Dall wrote:
I believe this includes all comments so far. I have also swatted up on
Documentation/CodingStyle, and modified the comments somewhat to
conform.
There is also a Perl script called scripts/checkpatch.pl that you
should run against your patch before submitting. It can identify and
report many common style problems.
Other than the two issues I mention below, I think this is now pretty
close.
I really hope this is acceptable now. One proviso. I don't actually
have
an IPv6 network so that code path is untested (except that it
compiles).
Unfortunately my IPv6 tunnel provider just dropped off the face of the
earth, so I currently don't have any IPv6 hosts to test this either.
I will have to figure something out quickly.
From: Ian Dall <ian@xxxxxxxxxxxxxxxxxxxxx>
Addresses: http://bugzilla.kernel.org/show_bug.cgi?id=11061
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. In the case of IPv6
sin6_flowinfo is not compared because it only affects QoS and
sin6_scope_id is only compared if the address is "link local" because
"link local" addresses need only be unique to a specific link.
Signed-off-by: Ian Dall <ian@xxxxxxxxxxxxxxxxxxxxx>
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 9b728f3..fa4060d 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -273,6 +273,65 @@ static int nfs_sockaddr_match_ipaddr(const
struct sockaddr *sa1,
#endif
/*
+ * Test if two ip4 socket addresses refer to the same socket, by
+ * comparing relevant fields. The padding bytes specifically, are
+ * not compared.
+ *
+ * The caller should ensure both socket addresses are AF_INET.
+ */
+static int nfs_sockaddr_cmp_ip4(const struct sockaddr_in * saddr1,
+ const struct sockaddr_in * saddr2)
+{
+ if (saddr1->sin_addr.s_addr != saddr2->sin_addr.s_addr)
+ return 0;
+ return saddr1->sin_port == saddr2->sin_port;
+}
+
+/*
+ * Test if two ip6 socket addresses refer to the same socket by
+ * comparing relevant fields. The padding bytes specifically, are not
+ * compared. sin6_flowinfo is not compared because it only affects
QoS
+ * and sin6_scope_id is only compared if the address is "link local"
+ * because "link local" addresses need only be unique to a specific
+ * link. Conversely, ordinary unicast addresses might have different
+ * sin6_scope_id.
+ *
+ * The caller should ensure both socket addresses are AF_INET6.
+ */
+static int nfs_sockaddr_cmp_ip6 (const struct sockaddr_in6 * saddr1,
+ const struct sockaddr_in6 * saddr2)
+{
+ if (!ipv6_addr_equal(&saddr1->sin6_addr,
+ &saddr1->sin6_addr))
+ return 0;
+ if (ipv6_addr_scope(&saddr1->sin6_addr) ==
IPV6_ADDR_SCOPE_LINKLOCAL &&
+ saddr1->sin6_scope_id != saddr2->sin6_scope_id)
+ return 0;
I've thought more about this. I think we don't want the scope ID
comparison here at all. The scope ID is not really part of an IPv6
address. A unique host address is contained only in the sin6_addr
field; the scope ID is simply local routing information. I can't
think of a case where two unique remotes would have identical link-
local addresses.
So I think we should remove the scope ID check entirely.
I haven't added support for scope IDs in other areas unless it is
needed for actually sending an RPC or reporting a problem.
nlm_cmp_addr() for example does not have this check, and neither does
nfs_sockaddr_match_ipaddr().
+ return saddr1->sin6_port == saddr2->sin6_port;
+}
+
+/*
+ * Test if two socket addresses represent the same actual socket,
+ * by comparing (only) relevant fields.
+ */
+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);
Just a style nit here: we usually leave out the blank between the
type cast and the variable these days.
In other areas, I think I placed the casts in the helpers just because
it makes the caller easier to read, but that's perhaps optional.
+ }
+ return 0;
+}
+
+/*
* Find a client by IP address and protocol version
* - returns NULL if no such client
*/
@@ -344,8 +403,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 +419,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);
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
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
