[PATCH 0/6] Refactor client_check()

[PATCH 0/6] Refactor client_check()

[Chuck Lever]

Hi Steve-

These patches refactor client_check() so that IPv6 support can be
added down the road.  They should introduce no change in behavior.

---

Chuck Lever (6):
      libexport.a: replace xlog(L_FATAL) in client_check()
      libexport.a: Refactor wildcard checking in client_check()
      libexport.a: Refactor netgroup checking in client_check()
      libexport.a: Remove unused function client_checkaddr()
      libexport.a: Factor SUBNETWORK checking out of check_client()
      libexport.a: Factor FQDN checking out of check_client()


 support/export/client.c |  204 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 130 insertions(+), 74 deletions(-)

-- 
Chuck Lever

[PATCH 1/6] libexport.a: Factor FQDN checking out of check_client()

[Chuck Lever]

Clean up:  Factor the MCL_FQDN case out of check_client() and
client_checkaddr().  This will make it easier to add IPv6 support
eventually.

The logic in the new helper function will get a little more tangled
once IPv6 support is introduced.  Each slot in the clp address list
eventually may contain an address from either address family.

Note that the switch statement in client_checkaddr() is redundant,
since clp->cl_mtype is loop invariant.  This change makes FQDN client
checking more computationally efficient, at the cost of a few extra
lines of code.

This commit should not change code behavior in any way.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index 5e937b0..833f4a9 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -329,6 +329,28 @@ add_name(char *old, const char *add)
 }
 
 /*
+ * Check each address listed in @hp against each address
+ * stored in @clp.  Return 1 if a match is found, otherwise
+ * zero.
+ */
+static int
+check_fqdn(const nfs_client *clp, const struct hostent *hp)
+{
+	struct in_addr addr;
+	char **ap;
+	int i;
+
+	for (ap = hp->h_addr_list; *ap; ap++) {
+		addr = *(struct in_addr *)*ap;
+
+		for (i = 0; i < clp->m_naddr; i++)
+			if (clp->m_addrlist[i].s_addr == addr.s_addr)
+				return 1;
+	}
+	return 0;
+}
+
+/*
  * Match a host (given its hostent record) to a client record. This
  * is usually called from mountd.
  */
@@ -341,6 +363,7 @@ client_check(nfs_client *clp, struct hostent *hp)
 
 	switch (clp->m_type) {
 	case MCL_FQDN:
+		return check_fqdn(clp, hp);
 	case MCL_SUBNETWORK:
 		for (ap = hp->h_addr_list; *ap; ap++) {
 			if (client_checkaddr(clp, *(struct in_addr *) *ap))
@@ -411,15 +434,7 @@ client_check(nfs_client *clp, struct hostent *hp)
 static int
 client_checkaddr(nfs_client *clp, struct in_addr addr)
 {
-	int	i;
-
 	switch (clp->m_type) {
-	case MCL_FQDN:
-		for (i = 0; i < clp->m_naddr; i++) {
-			if (clp->m_addrlist[i].s_addr == addr.s_addr)
-				return 1;
-		}
-		return 0;
 	case MCL_SUBNETWORK:
 		return !((clp->m_addrlist[0].s_addr ^ addr.s_addr)
 			& clp->m_addrlist[1].s_addr);

[PATCH 2/6] libexport.a: Factor SUBNETWORK checking out of check_client()

[Chuck Lever]

Clean up:  Factor the MCL_SUBNETWORK case out of check_client() and
client_checkaddr().  This will make it easier to add IPv6 support
eventually.

The logic in the new helper function will get a little more tangled
once IPv6 support is introduced.  Each slot in the clp address list
eventually may contain an address from either address family.

Note that the switch statement in client_checkaddr() is redundant,
since clp->cl_mtype is loop invariant.  This change makes SUBNETWORK
client checking more computationally efficient, at the cost of a few
extra lines of code.

This commit should not change code behavior in any way.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index 833f4a9..42bf45a 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -351,6 +351,27 @@ check_fqdn(const nfs_client *clp, const struct hostent *hp)
 }
 
 /*
+ * Check each address listed in @hp against the subnetwork or
+ * host address stored in @clp.  Return 1 if an address in @hp
+ * matches the host address stored in @clp, otherwise zero.
+ */
+static int
+check_subnetwork(const nfs_client *clp, const struct hostent *hp)
+{
+	struct in_addr addr;
+	char **ap;
+
+	for (ap = hp->h_addr_list; *ap; ap++) {
+		addr = *(struct in_addr *)*ap;
+
+		if (!((clp->m_addrlist[0].s_addr ^ addr.s_addr) &
+		      clp->m_addrlist[1].s_addr))
+			return 1;
+	}
+	return 0;
+}
+
+/*
  * Match a host (given its hostent record) to a client record. This
  * is usually called from mountd.
  */
@@ -365,11 +386,7 @@ client_check(nfs_client *clp, struct hostent *hp)
 	case MCL_FQDN:
 		return check_fqdn(clp, hp);
 	case MCL_SUBNETWORK:
-		for (ap = hp->h_addr_list; *ap; ap++) {
-			if (client_checkaddr(clp, *(struct in_addr *) *ap))
-				return 1;
-		}
-		return 0;
+		return check_subnetwork(clp, hp);
 	case MCL_WILDCARD:
 		if (wildmat(hname, cname))
 			return 1;
@@ -434,11 +451,6 @@ client_check(nfs_client *clp, struct hostent *hp)
 static int
 client_checkaddr(nfs_client *clp, struct in_addr addr)
 {
-	switch (clp->m_type) {
-	case MCL_SUBNETWORK:
-		return !((clp->m_addrlist[0].s_addr ^ addr.s_addr)
-			& clp->m_addrlist[1].s_addr);
-	}
 	return 0;
 }
 

[PATCH 3/6] libexport.a: Remove unused function client_checkaddr()

[Chuck Lever]

Clean up.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index 42bf45a..419a4a7 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -32,7 +32,6 @@ extern int	innetgr(char *netgr, char *host, char *, char *);
 static char	*add_name(char *old, const char *add);
 static void	client_init(nfs_client *clp, const char *hname,
 					struct hostent *hp);
-static int	client_checkaddr(nfs_client *clp, struct in_addr addr);
 
 nfs_client	*clientlist[MCL_MAXTYPES] = { NULL, };
 
@@ -448,12 +447,6 @@ client_check(nfs_client *clp, struct hostent *hp)
 	return 0;
 }
 
-static int
-client_checkaddr(nfs_client *clp, struct in_addr addr)
-{
-	return 0;
-}
-
 int
 client_gettype(char *ident)
 {

[PATCH 4/6] libexport.a: Refactor netgroup checking in client_check()

[Chuck Lever]

Clean up: refactor netgroup logic out of client_check() to make it
easier to introduce IPv6 support.

 +  Use preferred style of keeping #ifdef out of the middle of
    function definitions.  Squelch compiler warnings for "#ifndef
    HAVE_INNETGR" by using __attribute__((unused)).

 +  Use preferred style of not using curly braces around switch cases.

 +  Match style used for check_{fqdn,subnetwork}.

 +  Clarify comment documenting use of h_aliases

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |   97 ++++++++++++++++++++++++++++-------------------
 1 files changed, 57 insertions(+), 40 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index 419a4a7..580012d 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -371,6 +371,62 @@ check_subnetwork(const nfs_client *clp, const struct hostent *hp)
 }
 
 /*
+ * Check if @hp's hostname or aliases fall in a given netgroup.
+ * Return 1 if @hp represents a host in the netgroup, otherwise zero.
+ */
+#ifdef HAVE_INNETGR
+static int
+check_netgroup(const nfs_client *clp, const struct hostent *hp)
+{
+	const char *netgroup = clp->m_hostname + 1;
+	const char *hname = hp->h_name;
+	struct hostent *nhp = NULL;
+	struct sockaddr_in addr;
+	int match, i;
+	char *dot;
+
+	/* First, try to match the hostname without
+	 * splitting off the domain */
+	if (innetgr(netgroup, hname, NULL, NULL))
+		return 1;
+
+	/* See if hname aliases listed in /etc/hosts or nis[+]
+	 * match the requested netgroup */
+	for (i = 0; hp->h_aliases[i]; i++) {
+		if (innetgr(netgroup, hp->h_aliases[i], NULL, NULL))
+			return 1;
+	}
+
+	/* If hname is ip address convert to FQDN */
+	if (inet_aton(hname, &addr.sin_addr) &&
+	   (nhp = gethostbyaddr((const char *)&(addr.sin_addr),
+	    sizeof(addr.sin_addr), AF_INET))) {
+		hname = nhp->h_name;
+		if (innetgr(netgroup, hname, NULL, NULL))
+			return 1;
+	}
+
+	/* Okay, strip off the domain (if we have one) */
+	dot = strchr(hname, '.');
+	if (dot == NULL)
+		return 0;
+
+	*dot = '\0';
+	match = innetgr(netgroup, hname, NULL, NULL);
+	*dot = '.';
+
+	return match;
+}
+#else	/* !HAVE_INNETGR */
+static int
+check_netgroup(__attribute__((unused)) const nfs_client *clp,
+		__attribute__((unused)) const struct hostent *hp)
+{
+	return 0;
+}
+#endif	/* !HAVE_INNETGR */
+
+/*
  * Match a host (given its hostent record) to a client record. This
  * is usually called from mountd.
  */
@@ -396,46 +452,7 @@ client_check(nfs_client *clp, struct hostent *hp)
 		}
 		return 0;
 	case MCL_NETGROUP:
-#ifdef HAVE_INNETGR
-		{
-			char	*dot;
-			int	match, i;
-			struct hostent *nhp = NULL;
-			struct sockaddr_in addr;
-
-			/* First, try to match the hostname without
-			 * splitting off the domain */
-			if (innetgr(cname+1, hname, NULL, NULL))
-				return 1;
-
-			/* try the aliases as well */
-			for (i = 0; hp->h_aliases[i]; i++) {
-				if (innetgr(cname+1, hp->h_aliases[i], NULL, NULL))
-					return 1;
-			}
-
-			/* If hname is ip address convert to FQDN */
-			if (inet_aton(hname, &addr.sin_addr) &&
-			   (nhp = gethostbyaddr((const char *)&(addr.sin_addr),
-			    sizeof(addr.sin_addr), AF_INET))) {
-				hname = (char *)nhp->h_name;
-				if (innetgr(cname+1, hname, NULL, NULL))
-					return 1;
-			}
-
-			/* Okay, strip off the domain (if we have one) */
-			if ((dot = strchr(hname, '.')) == NULL)
-				return 0;
-
-			*dot = '\0';
-			match = innetgr(cname+1, hname, NULL, NULL);
-			*dot = '.';
-
-			return match;
-		}
-#else
-		return 0;
-#endif
+		return check_netgroup(clp, hp);
 	case MCL_ANONYMOUS:
 		return 1;
 	case MCL_GSS:

[PATCH 5/6] libexport.a: Refactor wildcard checking in client_check()

[Chuck Lever]

Clean up: refactor wildcard logic out of client_check() to make it
easier to introduce IPv6 support.

Match the style used for client_check_{fqdn,subnetwork,netgroup}.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index 580012d..bf2a359 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -371,6 +371,31 @@ check_subnetwork(const nfs_client *clp, const struct hostent *hp)
 }
 
 /*
+ * Check if a wildcard nfs_client record matches the canonical name
+ * or the aliases of a host.  Return 1 if a match is found, otherwise
+ * zero.
+ */
+static int
+check_wildcard(const nfs_client *clp, const struct hostent *hp)
+{
+	char *cname = clp->m_hostname;
+	char *hname = hp->h_name;
+	char **ap;
+
+	if (wildmat(hname, cname))
+		return 1;
+
+	/* See if hname aliases listed in /etc/hosts or nis[+]
+	 * match the requested wildcard */
+	for (ap = hp->h_aliases; *ap; ap++) {
+		if (wildmat(*ap, cname))
+			return 1;
+	}
+
+	return 0;
+}
+
+/*
  * Check if @hp's hostname or aliases fall in a given netgroup.
  * Return 1 if @hp represents a host in the netgroup, otherwise zero.
  */
@@ -433,24 +458,13 @@ check_netgroup(__attribute__((unused)) const nfs_client *clp,
 int
 client_check(nfs_client *clp, struct hostent *hp)
 {
-	char	*hname = (char *) hp->h_name;
-	char	*cname = clp->m_hostname;
-	char	**ap;
-
 	switch (clp->m_type) {
 	case MCL_FQDN:
 		return check_fqdn(clp, hp);
 	case MCL_SUBNETWORK:
 		return check_subnetwork(clp, hp);
 	case MCL_WILDCARD:
-		if (wildmat(hname, cname))
-			return 1;
-		else {
-			for (ap = hp->h_aliases; *ap; ap++)
-				if (wildmat(*ap, cname))
-					return 1;
-		}
-		return 0;
+		return check_wildcard(clp, hp);
 	case MCL_NETGROUP:
 		return check_netgroup(clp, hp);
 	case MCL_ANONYMOUS:

[PATCH 6/6] libexport.a: replace xlog(L_FATAL) in client_check()

[Chuck Lever]

Clean up: Prevent the process from exiting in client_check().  Report
bad m_type values, but return 0.

This removes another site where a mountd or exportfs process can exit
sideways.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/export/client.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index bf2a359..aa28fcf 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -451,9 +451,13 @@ check_netgroup(__attribute__((unused)) const nfs_client *clp,
 }
 #endif	/* !HAVE_INNETGR */
 
-/*
- * Match a host (given its hostent record) to a client record. This
- * is usually called from mountd.
+/**
+ * client_check - check if IP address information matches a cached nfs_client
+ * @clp: pointer to a cached nfs_client record
+ * @hp: pointer to hostent containing host IP information
+ *
+ * Returns 1 if the address information matches the cached nfs_client,
+ * otherwise zero.
  */
 int
 client_check(nfs_client *clp, struct hostent *hp)
@@ -472,7 +476,8 @@ client_check(nfs_client *clp, struct hostent *hp)
 	case MCL_GSS:
 		return 0;
 	default:
-		xlog(L_FATAL, "internal: bad client type %d", clp->m_type);
+		xlog(D_GENERAL, "%s: unrecognized client type: %d",
+				__func__, clp->m_type);
 	}
 
 	return 0;

Re: [PATCH 0/6] Refactor client_check()

[Steve Dickson]

On 03/23/2010 12:52 PM, Chuck Lever wrote:
> Hi Steve-
> 
> These patches refactor client_check() so that IPv6 support can be
> added down the road.  They should introduce no change in behavior.
> 
> ---
> 
> Chuck Lever (6):
>       libexport.a: replace xlog(L_FATAL) in client_check()
>       libexport.a: Refactor wildcard checking in client_check()
>       libexport.a: Refactor netgroup checking in client_check()
>       libexport.a: Remove unused function client_checkaddr()
>       libexport.a: Factor SUBNETWORK checking out of check_client()
>       libexport.a: Factor FQDN checking out of check_client()
> 
> 
>  support/export/client.c |  204 ++++++++++++++++++++++++++++++-----------------
>  1 files changed, 130 insertions(+), 74 deletions(-)
> 
Committed... 

steved.