[PATCH 0/3] AF_INET6 support for probe_bothports()

[PATCH 0/3] AF_INET6 support for probe_bothports()

[Chuck Lever]

Hi Steve-

Here's the next step.

This small series of patches rewires probe_bothports() to support
AF_INET6 addresses, now that the underlying functions can handle them.

Since legacy code in other parts of the mount command still use
probe_bothports() and the clnt_addr_t data type, I've added a new
function call to do the IPv6 duties.  The old API still exists and
continues to support only AF_INET, but under the covers it calls the
new code.

Again, for the time being, this is used only for the legacy binary
mount(2) interface.  We will need this for umount later, and that is
used to support both binary and text-based mounts.

---

Chuck Lever (3):
      mount command: AF_INET6 support for probe_bothports()
      mount command: support AF_INET6 in probe_nfsport() and probe_mntport()
      mount command: full support for AF_INET6 addresses in probe_port()


 utils/mount/network.c |  178 +++++++++++++++++++++++++++++++++++++++----------
 utils/mount/network.h |    3 +
 2 files changed, 144 insertions(+), 37 deletions(-)

-- 
Chuck Lever

[PATCH 1/3] mount command: full support for AF_INET6 addresses in probe_port()

[Chuck Lever]

Now that probe_port() uses an AF_INET6-capable rpcbind query and RPC ping,
finish updating probe_port() to support AF_INET6 addresses fully.

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

 utils/mount/network.c |   54 ++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 0c68993..e50bc28 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -160,6 +160,21 @@ static const unsigned long probe_mnt3_first[] = {
 	0,
 };
 
+static void nfs_set_port(struct sockaddr *sap, const unsigned short port)
+{
+	switch (sap->sa_family) {
+	case AF_INET:
+		((struct sockaddr_in *)sap)->sin_port = htons(port);
+		break;
+	case AF_INET6:
+		((struct sockaddr_in6 *)sap)->sin6_port = htons(port);
+		break;
+	default:
+		nfs_error(_("%s: unrecognized address family in %s"),
+			progname, __func__);
+	}
+}
+
 /**
  * nfs_name_to_address - resolve hostname to an IPv4 or IPv6 socket address
  * @hostname: pointer to C string containing DNS hostname to resolve
@@ -474,27 +489,38 @@ static void nfs_pp_debug(const struct sockaddr *sap, const socklen_t salen,
  * Use the portmapper to discover whether or not the service we want is
  * available. The lists 'versions' and 'protos' define ordered sequences
  * of service versions and udp/tcp protocols to probe for.
+ *
+ * Returns 1 if the requested service port is unambiguous and pingable;
+ * @pmap is filled in with the version, port, and transport protocol used
+ * during the successful ping.  Note that if a port is already specified
+ * in @pmap and it matches the rpcbind query result, nfs_probe_port() does
+ * not perform an RPC ping.
+ * 
+ * If an error occurs or the requested service isn't available, zero is
+ * returned; rpccreateerr.cf_stat is set to reflect the nature of the error.
  */
-static int probe_port(clnt_addr_t *server, const unsigned long *versions,
-			const unsigned int *protos)
+static int nfs_probe_port(const struct sockaddr *sap, const socklen_t salen,
+			  struct pmap *pmap, const unsigned long *versions,
+			  const unsigned int *protos)
 {
-	const struct sockaddr *saddr = (struct sockaddr *)&server->saddr;
-	const socklen_t salen = sizeof(server->saddr);
-	struct pmap *pmap = &server->pmap;
+	struct sockaddr_storage address;
+	struct sockaddr *saddr = (struct sockaddr *)&address;
 	const unsigned long prog = pmap->pm_prog, *p_vers;
 	const unsigned int prot = (u_int)pmap->pm_prot, *p_prot;
 	const u_short port = (u_short) pmap->pm_port;
 	unsigned long vers = pmap->pm_vers;
 	unsigned short p_port;
 
+	memcpy(saddr, sap, salen);
 	p_prot = prot ? &prot : protos;
 	p_vers = vers ? &vers : versions;
 	rpc_createerr.cf_stat = 0;
+
 	for (;;) {
 		p_port = nfs_getport(saddr, salen, prog, *p_vers, *p_prot);
 		if (p_port) {
 			if (!port || port == p_port) {
-				server->saddr.sin_port = htons(p_port);
+				nfs_set_port(saddr, p_port);
 				nfs_pp_debug(saddr, salen, prog, *p_vers,
 						*p_prot, p_port);
 				if (nfs_rpc_ping(saddr, salen, prog,
@@ -537,28 +563,36 @@ out_ok:
 
 static int probe_nfsport(clnt_addr_t *nfs_server)
 {
+	struct sockaddr *sap = (struct sockaddr *)&nfs_server->saddr;
+	socklen_t salen = sizeof(nfs_server->saddr);
 	struct pmap *pmap = &nfs_server->pmap;
 
 	if (pmap->pm_vers && pmap->pm_prot && pmap->pm_port)
 		return 1;
 
 	if (nfs_mount_data_version >= 4)
-		return probe_port(nfs_server, probe_nfs3_first, probe_tcp_first);
+		return nfs_probe_port(sap, salen, pmap,
+					probe_nfs3_first, probe_tcp_first);
 	else
-		return probe_port(nfs_server, probe_nfs2_only, probe_udp_only);
+		return nfs_probe_port(sap, salen, pmap,
+					probe_nfs2_only, probe_udp_only);
 }
 
 static int probe_mntport(clnt_addr_t *mnt_server)
 {
+	struct sockaddr *sap = (struct sockaddr *)&mnt_server->saddr;
+	socklen_t salen = sizeof(mnt_server->saddr);
 	struct pmap *pmap = &mnt_server->pmap;
 
 	if (pmap->pm_vers && pmap->pm_prot && pmap->pm_port)
 		return 1;
 
 	if (nfs_mount_data_version >= 4)
-		return probe_port(mnt_server, probe_mnt3_first, probe_udp_first);
+		return nfs_probe_port(sap, salen, pmap,
+					probe_mnt3_first, probe_udp_first);
 	else
-		return probe_port(mnt_server, probe_mnt1_first, probe_udp_only);
+		return nfs_probe_port(sap, salen, pmap,
+					probe_mnt1_first, probe_udp_only);
 }
 
 /**

[PATCH 2/3] mount command: support AF_INET6 in probe_nfsport() and probe_mntport()

[Chuck Lever]

Flesh out support for AF_INET6 in the intermediate helper functions
probe_nfsport() and probe_mntport().

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

 utils/mount/network.c |   58 +++++++++++++++++++++++++++++++++++--------------
 1 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index e50bc28..55b2cab 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -561,12 +561,22 @@ out_ok:
 	return 1;
 }
 
-static int probe_nfsport(clnt_addr_t *nfs_server)
+/*
+ * Probe a server's NFS service to determine which versions and
+ * transport protocols are supported.
+ *
+ * Returns 1 if the requested service port is unambiguous and pingable;
+ * @pmap is filled in with the version, port, and transport protocol used
+ * during the successful ping.  If all three are already specified, simply
+ * return success without an rpcbind query or RPC ping (we may be trying
+ * to mount an NFS service that is not advertised via rpcbind).
+ *
+ * If an error occurs or the requested service isn't available, zero is
+ * returned; rpccreateerr.cf_stat is set to reflect the nature of the error.
+ */
+static int nfs_probe_nfsport(const struct sockaddr *sap, const socklen_t salen,
+				struct pmap *pmap)
 {
-	struct sockaddr *sap = (struct sockaddr *)&nfs_server->saddr;
-	socklen_t salen = sizeof(nfs_server->saddr);
-	struct pmap *pmap = &nfs_server->pmap;
-
 	if (pmap->pm_vers && pmap->pm_prot && pmap->pm_port)
 		return 1;
 
@@ -578,12 +588,22 @@ static int probe_nfsport(clnt_addr_t *nfs_server)
 					probe_nfs2_only, probe_udp_only);
 }
 
-static int probe_mntport(clnt_addr_t *mnt_server)
+/*
+ * Probe a server's mountd service to determine which versions and
+ * transport protocols are supported.
+ *
+ * Returns 1 if the requested service port is unambiguous and pingable;
+ * @pmap is filled in with the version, port, and transport protocol used
+ * during the successful ping.  If all three are already specified, simply
+ * return success without an rpcbind query or RPC ping (we may be trying
+ * to mount an NFS service that is not advertised via rpcbind).
+ * 
+ * If an error occurs or the requested service isn't available, zero is
+ * returned; rpccreateerr.cf_stat is set to reflect the nature of the error.
+ */
+static int nfs_probe_mntport(const struct sockaddr *sap, const socklen_t salen,
+				struct pmap *pmap)
 {
-	struct sockaddr *sap = (struct sockaddr *)&mnt_server->saddr;
-	socklen_t salen = sizeof(mnt_server->saddr);
-	struct pmap *pmap = &mnt_server->pmap;
-
 	if (pmap->pm_vers && pmap->pm_prot && pmap->pm_port)
 		return 1;
 
@@ -607,10 +627,13 @@ static int probe_mntport(clnt_addr_t *mnt_server)
  */
 int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 {
+	struct sockaddr *nfs_saddr = (struct sockaddr *)&nfs_server->saddr;
+	socklen_t nfs_salen = sizeof(nfs_server->saddr);
+	struct sockaddr *mnt_saddr = (struct sockaddr *)&mnt_server->saddr;
+	socklen_t mnt_salen = sizeof(mnt_server->saddr);
 	struct pmap *nfs_pmap = &nfs_server->pmap;
 	struct pmap *mnt_pmap = &mnt_server->pmap;
 	struct pmap save_nfs, save_mnt;
-	int res;
 	const unsigned long *probe_vers;
 
 	if (mnt_pmap->pm_vers && !nfs_pmap->pm_vers)
@@ -627,9 +650,9 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 
 	for (; *probe_vers; probe_vers++) {
 		nfs_pmap->pm_vers = mntvers_to_nfs(*probe_vers);
-		if ((res = probe_nfsport(nfs_server) != 0)) {
+		if (nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap) != 0) {
 			mnt_pmap->pm_vers = *probe_vers;
-			if ((res = probe_mntport(mnt_server)) != 0)
+			if (nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap) != 0)
 				return 1;
 			memcpy(mnt_pmap, &save_mnt, sizeof(*mnt_pmap));
 		}
@@ -647,9 +670,9 @@ out_bad:
 	return 0;
 
 version_fixed:
-	if (!probe_nfsport(nfs_server))
+	if (!nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap))
 		goto out_bad;
-	return probe_mntport(mnt_server);
+	return nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap);
 }
 
 static int nfs_probe_statd(void)
@@ -716,11 +739,14 @@ int start_statd(void)
  */
 int nfs_call_umount(clnt_addr_t *mnt_server, dirpath *argp)
 {
+	struct sockaddr *sap = (struct sockaddr *)&mnt_server->saddr;
+	socklen_t salen = sizeof(mnt_server->saddr);
+	struct pmap *pmap = &mnt_server->pmap;
 	CLIENT *clnt;
 	enum clnt_stat res = 0;
 	int msock;
 
-	if (!probe_mntport(mnt_server))
+	if (!nfs_probe_mntport(sap, salen, pmap))
 		return 0;
 	clnt = mnt_openclnt(mnt_server, &msock);
 	if (!clnt)

[PATCH 3/3] mount command: AF_INET6 support for probe_bothports()

[Chuck Lever]

Introduce an AF_INET6 capable probe_bothports() API.  This means replacing
"struct sockaddr_in *" arguments with a "struct sockaddr *" and a socklen_t
arguments.

These functions often combine a "struct sockaddr_in" and a "struct pmap" into
a single "clnt_addr_t" argument.  Instead of modifying "clnt_addr_t" and all
the legacy code that uses it, I'm going to create a new probe_bothports() API
for the text-based mount command that takes a "struct sockaddr *" and
sockaddr length, and leave the existing probe_bothports() interface, which
takes "clnt_addr_t" arguments, for legacy use.

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

 utils/mount/network.c |   86 +++++++++++++++++++++++++++++++++++++------------
 utils/mount/network.h |    3 ++
 2 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 55b2cab..6a9a41a 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -615,24 +615,49 @@ static int nfs_probe_mntport(const struct sockaddr *sap, const socklen_t salen,
 					probe_mnt1_first, probe_udp_only);
 }
 
-/**
- * probe_bothports - discover the RPC endpoints of mountd and NFS server
- * @mnt_server: pointer to address and pmap argument for mountd results
- * @nfs_server: pointer to address and pmap argument for NFS server
+/*
+ * Probe a server's mountd service to determine which versions and
+ * transport protocols are supported.  Invoked when the protocol
+ * version is already known for both the NFS and mountd service.
  *
- * Returns 1 if successful, otherwise zero if some error occurred.
- * Note that the arguments are both input and output arguments.
+ * Returns 1 and fills in both @pmap structs if the requested service
+ * ports are unambiguous and pingable.  Otherwise zero is returned;
+ * rpccreateerr.cf_stat is set to reflect the nature of the error.
+ */
+static int nfs_probe_version_fixed(const struct sockaddr *mnt_saddr,
+			const socklen_t mnt_salen,
+			struct pmap *mnt_pmap,
+			const struct sockaddr *nfs_saddr,
+			const socklen_t nfs_salen,
+			struct pmap *nfs_pmap)
+{
+	if (!nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap))
+		return 0;
+	return nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap);
+}
+
+/**
+ * nfs_probe_bothports - discover the RPC endpoints of mountd and NFS server
+ * @mnt_saddr:	pointer to socket address of mountd server
+ * @mnt_salen:	length of mountd server's address
+ * @mnt_pmap:	IN: partially filled-in mountd RPC service tuple;
+ *		OUT: fully filled-in mountd RPC service tuple
+ * @nfs_saddr:	pointer to socket address of NFS server
+ * @nfs_salen:	length of NFS server's address
+ * @nfs_pmap:	IN: partially filled-in NFS RPC service tuple;
+ *		OUT: fully filled-in NFS RPC service tuple
  *
- * A side effect of calling this function is that rpccreateerr is set.
+ * Returns 1 and fills in both @pmap structs if the requested service
+ * ports are unambiguous and pingable.  Otherwise zero is returned;
+ * rpccreateerr.cf_stat is set to reflect the nature of the error.
  */
-int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
+int nfs_probe_bothports(const struct sockaddr *mnt_saddr,
+			const socklen_t mnt_salen,
+			struct pmap *mnt_pmap,
+			const struct sockaddr *nfs_saddr,
+			const socklen_t nfs_salen,
+			struct pmap *nfs_pmap)
 {
-	struct sockaddr *nfs_saddr = (struct sockaddr *)&nfs_server->saddr;
-	socklen_t nfs_salen = sizeof(nfs_server->saddr);
-	struct sockaddr *mnt_saddr = (struct sockaddr *)&mnt_server->saddr;
-	socklen_t mnt_salen = sizeof(mnt_server->saddr);
-	struct pmap *nfs_pmap = &nfs_server->pmap;
-	struct pmap *mnt_pmap = &mnt_server->pmap;
 	struct pmap save_nfs, save_mnt;
 	const unsigned long *probe_vers;
 
@@ -640,8 +665,10 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 		nfs_pmap->pm_vers = mntvers_to_nfs(mnt_pmap->pm_vers);
 	else if (nfs_pmap->pm_vers && !mnt_pmap->pm_vers)
 		mnt_pmap->pm_vers = nfsvers_to_mnt(nfs_pmap->pm_vers);
+
 	if (nfs_pmap->pm_vers)
-		goto version_fixed;
+		return nfs_probe_version_fixed(mnt_saddr, mnt_salen, mnt_pmap,
+					       nfs_saddr, nfs_salen, nfs_pmap);
 
 	memcpy(&save_nfs, nfs_pmap, sizeof(save_nfs));
 	memcpy(&save_mnt, mnt_pmap, sizeof(save_mnt));
@@ -661,18 +688,35 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 		case RPC_PROGNOTREGISTERED:
 			break;
 		default:
-			goto out_bad;
+			return 0;
 		}
 		memcpy(nfs_pmap, &save_nfs, sizeof(*nfs_pmap));
 	}
 
-out_bad:
 	return 0;
+}
 
-version_fixed:
-	if (!nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap))
-		goto out_bad;
-	return nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap);
+/**
+ * probe_bothports - discover the RPC endpoints of mountd and NFS server
+ * @mnt_server: pointer to address and pmap argument for mountd results
+ * @nfs_server: pointer to address and pmap argument for NFS server
+ *
+ * This is the legacy API that takes "clnt_addr_t" for both servers,
+ * but supports only AF_INET addresses.
+ *
+ * Returns 1 and fills in the pmap field in both clnt_addr_t structs
+ * if the requested service ports are unambiguous and pingable.
+ * Otherwise zero is returned; rpccreateerr.cf_stat is set to reflect
+ * the nature of the error.
+ */
+int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
+{
+	return nfs_probe_bothports((struct sockaddr *)&mnt_server->saddr,
+					sizeof(mnt_server->saddr),
+					&mnt_server->pmap,
+					(struct sockaddr *)&nfs_server->saddr,
+					sizeof(nfs_server->saddr),
+					&nfs_server->pmap);
 }
 
 static int nfs_probe_statd(void)
diff --git a/utils/mount/network.h b/utils/mount/network.h
index a4dba1b..075093d 100644
--- a/utils/mount/network.h
+++ b/utils/mount/network.h
@@ -40,6 +40,9 @@ static const struct timeval TIMEOUT = { 20, 0 };
 static const struct timeval RETRY_TIMEOUT = { 3, 0 };
 
 int probe_bothports(clnt_addr_t *, clnt_addr_t *);
+int nfs_probe_bothports(const struct sockaddr *, const socklen_t,
+			struct pmap *, const struct sockaddr *,
+			const socklen_t, struct pmap *);
 int nfs_gethostbyname(const char *, struct sockaddr_in *);
 int nfs_name_to_address(const char *, const sa_family_t,
 		struct sockaddr *, socklen_t *);

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steve Dickson]

Chuck Lever wrote:
> Hi Steve-
> 
> Here's the next step.
> 
> This small series of patches rewires probe_bothports() to support
> AF_INET6 addresses, now that the underlying functions can handle them.
> 
> Since legacy code in other parts of the mount command still use
> probe_bothports() and the clnt_addr_t data type, I've added a new
> function call to do the IPv6 duties.  The old API still exists and
> continues to support only AF_INET, but under the covers it calls the
> new code.
> 
> Again, for the time being, this is used only for the legacy binary
> mount(2) interface.  We will need this for umount later, and that is
> used to support both binary and text-based mounts.
> 
> ---
> 
> Chuck Lever (3):
>       mount command: AF_INET6 support for probe_bothports()
>       mount command: support AF_INET6 in probe_nfsport() and probe_mntport()
>       mount command: full support for AF_INET6 addresses in probe_port()
> 
Question: in the clnt_addr_t typedef:

typedef struct {
    char **hostname;
    struct sockaddr_in saddr;
    struct pmap pmap; 
} clnt_addr_t;

Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?

It seems at the beginning of each routine saddr is always being
typecast into a struct sockaddr pointer.... So wouldn't be easier
and cleaner to simply make sadd a struct sockaddr or am I missing
something?

steved.

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steinar H. Gunderson]

On Mon, Dec 08, 2008 at 08:46:36AM -0500, Steve Dickson wrote:
> typedef struct {
>     char **hostname;
>     struct sockaddr_in saddr;
>     struct pmap pmap; 
> } clnt_addr_t;
> 
> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?
> 
> It seems at the beginning of each routine saddr is always being
> typecast into a struct sockaddr pointer.... So wouldn't be easier
> and cleaner to simply make sadd a struct sockaddr or am I missing
> something?

I'm sure most people here know this, but still, be careful: If you ever
intend to store an IPv6 address somewhere, the field in the struct should be
a sockaddr_storage. sockaddr (or sockaddr_in, for that matter) is not big
enough to store a sockaddr_in6.

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steve Dickson]

Steinar H. Gunderson wrote:
> On Mon, Dec 08, 2008 at 08:46:36AM -0500, Steve Dickson wrote:
>> typedef struct {
>>     char **hostname;
>>     struct sockaddr_in saddr;
>>     struct pmap pmap; 
>> } clnt_addr_t;
>>
>> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?
>>
>> It seems at the beginning of each routine saddr is always being
>> typecast into a struct sockaddr pointer.... So wouldn't be easier
>> and cleaner to simply make sadd a struct sockaddr or am I missing
>> something?
> 
> I'm sure most people here know this, but still, be careful: If you ever
> intend to store an IPv6 address somewhere, the field in the struct should be
> a sockaddr_storage. sockaddr (or sockaddr_in, for that matter) is not big
> enough to store a sockaddr_in6.
Understood... But isn't a struct sockaddr large enough to store
a IPv6 address? Its no biggie... but what caught my eye was usually 
struct sockaddr are typecast into sockaddr_in, not the other away around.

steved.

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Chuck Lever]

On Dec 8, 2008, at Dec 8, 2008, 2:26 PM, Steve Dickson wrote:
> Steinar H. Gunderson wrote:
>> On Mon, Dec 08, 2008 at 08:46:36AM -0500, Steve Dickson wrote:
>>> typedef struct {
>>>    char **hostname;
>>>    struct sockaddr_in saddr;
>>>    struct pmap pmap;
>>> } clnt_addr_t;
>>>
>>> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?
>>>
>>> It seems at the beginning of each routine saddr is always being
>>> typecast into a struct sockaddr pointer.... So wouldn't be easier
>>> and cleaner to simply make sadd a struct sockaddr or am I missing
>>> something?
>>
>> I'm sure most people here know this, but still, be careful: If you  
>> ever
>> intend to store an IPv6 address somewhere, the field in the struct  
>> should be
>> a sockaddr_storage. sockaddr (or sockaddr_in, for that matter) is  
>> not big
>> enough to store a sockaddr_in6.
> Understood... But isn't a struct sockaddr large enough to store
> a IPv6 address?

No, they aren't: IPv6 addresses are larger than sockaddr_in's.   
sockaddr's happen to be the same size as sockaddr_in's.  I think this  
is defined by POSIX, but I could be incorrect.

> Its no biggie... but what caught my eye was usually
> struct sockaddr are typecast into sockaddr_in, not the other away  
> around.


-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Chuck Lever]

On Dec 8, 2008, at Dec 8, 2008, 8:46 AM, Steve Dickson wrote:
> Chuck Lever wrote:
>> Hi Steve-
>>
>> Here's the next step.
>>
>> This small series of patches rewires probe_bothports() to support
>> AF_INET6 addresses, now that the underlying functions can handle  
>> them.
>>
>> Since legacy code in other parts of the mount command still use
>> probe_bothports() and the clnt_addr_t data type, I've added a new
>> function call to do the IPv6 duties.  The old API still exists and
>> continues to support only AF_INET, but under the covers it calls the
>> new code.
>>
>> Again, for the time being, this is used only for the legacy binary
>> mount(2) interface.  We will need this for umount later, and that is
>> used to support both binary and text-based mounts.
>>
>> ---
>>
>> Chuck Lever (3):
>>      mount command: AF_INET6 support for probe_bothports()
>>      mount command: support AF_INET6 in probe_nfsport() and  
>> probe_mntport()
>>      mount command: full support for AF_INET6 addresses in  
>> probe_port()
>>
> Question: in the clnt_addr_t typedef:
>
> typedef struct {
>    char **hostname;
>    struct sockaddr_in saddr;
>    struct pmap pmap;
> } clnt_addr_t;
>
> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?

Conventionally, "struct sockaddr" is supposed to be used only for  
pointers, not for declaring storage to store addresses.  sockaddr_in  
can be used for either declaring storage or for a pointer declaration.

What is defined here is a "struct sockaddr_in", not a pointer.

If we wanted to store a generic address rather than a pointer, the  
convention is to use struct sockaddr_storage, which is large enough to  
store an IPv4, IPv6 or even a Unix address.  But that would change the  
offsets of the following fields in clnt_addr_t, and that would break  
other legacy mount code.

> It seems at the beginning of each routine saddr is always being
> typecast into a struct sockaddr pointer....

More likely it is:

   struct sockaddr *sap = (struct sockaddr *)&saddr;

Which is appropriate and normal.

> So wouldn't be easier
> and cleaner to simply make saddr a struct sockaddr or am I missing
> something?

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steve Dickson]

Chuck Lever wrote:
> On Dec 8, 2008, at Dec 8, 2008, 8:46 AM, Steve Dickson wrote:
>> Chuck Lever wrote:
>>> Hi Steve-
>>>
>>> Here's the next step.
>>>
>>> This small series of patches rewires probe_bothports() to support
>>> AF_INET6 addresses, now that the underlying functions can handle them.
>>>
>>> Since legacy code in other parts of the mount command still use
>>> probe_bothports() and the clnt_addr_t data type, I've added a new
>>> function call to do the IPv6 duties.  The old API still exists and
>>> continues to support only AF_INET, but under the covers it calls the
>>> new code.
>>>
>>> Again, for the time being, this is used only for the legacy binary
>>> mount(2) interface.  We will need this for umount later, and that is
>>> used to support both binary and text-based mounts.
>>>
>>> ---
>>>
>>> Chuck Lever (3):
>>>      mount command: AF_INET6 support for probe_bothports()
>>>      mount command: support AF_INET6 in probe_nfsport() and
>>> probe_mntport()
>>>      mount command: full support for AF_INET6 addresses in probe_port()
>>>
>> Question: in the clnt_addr_t typedef:
>>
>> typedef struct {
>>    char **hostname;
>>    struct sockaddr_in saddr;
>>    struct pmap pmap;
>> } clnt_addr_t;
>>
>> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?
> 
> Conventionally, "struct sockaddr" is supposed to be used only for
> pointers, not for declaring storage to store addresses.  sockaddr_in can
> be used for either declaring storage or for a pointer declaration.
Yes... one does pass pointers of struct sockaddr to the majority 
of the network system call such as bind().. but conventionally 
I've seen a lot of declare struct sockaddr as memory then typecasting 
that memory into a struct sockaddr_in pointer...

> 
> What is defined here is a "struct sockaddr_in", not a pointer.
Understood...

> 
> If we wanted to store a generic address rather than a pointer, the
> convention is to use struct sockaddr_storage, which is large enough to
> store an IPv4, IPv6 or even a Unix address.  But that would change the
> offsets of the following fields in clnt_addr_t, and that would break
> other legacy mount code.
> 
>> It seems at the beginning of each routine saddr is always being
>> typecast into a struct sockaddr pointer....
> 
> More likely it is:
> 
>   struct sockaddr *sap = (struct sockaddr *)&saddr;
> 
> Which is appropriate and normal.
More to the point, I see a number of

    struct sockaddr *nfs_saddr = (struct sockaddr *)&nfs_server->saddr;

at the top of routines in the patch set you recently posted. To me it
seems like it would make more sense if saddr would was a struct sockaddr
to start with instead of always doing those typecasts... but its truly
just a nit... 

steved.

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steinar H. Gunderson]

On Mon, Dec 08, 2008 at 05:55:40PM -0500, Steve Dickson wrote:
> Yes... one does pass pointers of struct sockaddr to the majority 
> of the network system call such as bind().. but conventionally 
> I've seen a lot of declare struct sockaddr as memory then typecasting 
> that memory into a struct sockaddr_in pointer...

That's just wrong. Don't do that :-) (Where have you seen this, by the way?)

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Chuck Lever]

On Dec 8, 2008, at Dec 8, 2008, 6:28 PM, Steinar H. Gunderson wrote:
> On Mon, Dec 08, 2008 at 05:55:40PM -0500, Steve Dickson wrote:
>> Yes... one does pass pointers of struct sockaddr to the majority
>> of the network system call such as bind().. but conventionally
>> I've seen a lot of declare struct sockaddr as memory then typecasting
>> that memory into a struct sockaddr_in pointer...
>
> That's just wrong. Don't do that :-) (Where have you seen this, by  
> the way?)

The server-side sysctl code defines a data structure shared between  
nfs-utils and the kernel which has some fields defined as type "struct  
sockaddr".  That code works because "struct sockaddr_in" and "struct  
sockaddr" happen to be the same size.

But it is incorrect.  Since those are legacy interfaces and only need  
to handle IPv4 addresses, they are not worth fixing at this point.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steve Dickson]

Steinar H. Gunderson wrote:
> On Mon, Dec 08, 2008 at 05:55:40PM -0500, Steve Dickson wrote:
>> Yes... one does pass pointers of struct sockaddr to the majority 
>> of the network system call such as bind().. but conventionally 
>> I've seen a lot of declare struct sockaddr as memory then typecasting 
>> that memory into a struct sockaddr_in pointer...
> 
> That's just wrong. Don't do that :-) (Where have you seen this, by the way?)
I believe all over the RPC code in the glibc... for one place...
I have not looked at that code for a while so maybe I have
it backwards... 

steved.

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Trond Myklebust]

On Tue, 2008-12-09 at 14:17 -0500, Steve Dickson wrote:
> 
> Steinar H. Gunderson wrote:
> > On Mon, Dec 08, 2008 at 05:55:40PM -0500, Steve Dickson wrote:
> >> Yes... one does pass pointers of struct sockaddr to the majority 
> >> of the network system call such as bind().. but conventionally 
> >> I've seen a lot of declare struct sockaddr as memory then typecasting 
> >> that memory into a struct sockaddr_in pointer...
> > 
> > That's just wrong. Don't do that :-) (Where have you seen this, by the way?)
> I believe all over the RPC code in the glibc... for one place...
> I have not looked at that code for a while so maybe I have
> it backwards... 

No. That sounds correct. struct sockaddr as defined both in the kernel
and in glibc deliberately contains 14 bytes of padding. The struct
sockaddr_in is padded so that it is exactly the same size as struct
sockaddr.

They are not, however, large enough to contain a struct sockaddr_in6.

Cheers
  Trond

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Chuck Lever]

On Dec 8, 2008, at Dec 8, 2008, 5:55 PM, Steve Dickson wrote:
> Chuck Lever wrote:
>> On Dec 8, 2008, at Dec 8, 2008, 8:46 AM, Steve Dickson wrote:
>>> Chuck Lever wrote:
>>>> Hi Steve-
>>>>
>>>> Here's the next step.
>>>>
>>>> This small series of patches rewires probe_bothports() to support
>>>> AF_INET6 addresses, now that the underlying functions can handle  
>>>> them.
>>>>
>>>> Since legacy code in other parts of the mount command still use
>>>> probe_bothports() and the clnt_addr_t data type, I've added a new
>>>> function call to do the IPv6 duties.  The old API still exists and
>>>> continues to support only AF_INET, but under the covers it calls  
>>>> the
>>>> new code.
>>>>
>>>> Again, for the time being, this is used only for the legacy binary
>>>> mount(2) interface.  We will need this for umount later, and that  
>>>> is
>>>> used to support both binary and text-based mounts.
>>>>
>>>> ---
>>>>
>>>> Chuck Lever (3):
>>>>     mount command: AF_INET6 support for probe_bothports()
>>>>     mount command: support AF_INET6 in probe_nfsport() and
>>>> probe_mntport()
>>>>     mount command: full support for AF_INET6 addresses in  
>>>> probe_port()
>>>>
>>> Question: in the clnt_addr_t typedef:
>>>
>>> typedef struct {
>>>   char **hostname;
>>>   struct sockaddr_in saddr;
>>>   struct pmap pmap;
>>> } clnt_addr_t;
>>>
>>> Why isn't saddr a struct sockaddr instead of a struct sockaddr_in?
>>
>> Conventionally, "struct sockaddr" is supposed to be used only for
>> pointers, not for declaring storage to store addresses.   
>> sockaddr_in can
>> be used for either declaring storage or for a pointer declaration.
> Yes... one does pass pointers of struct sockaddr to the majority
> of the network system call such as bind().. but conventionally
> I've seen a lot of declare struct sockaddr as memory then typecasting
> that memory into a struct sockaddr_in pointer...
>
>>
>> What is defined here is a "struct sockaddr_in", not a pointer.
> Understood...
>
>>
>> If we wanted to store a generic address rather than a pointer, the
>> convention is to use struct sockaddr_storage, which is large enough  
>> to
>> store an IPv4, IPv6 or even a Unix address.  But that would change  
>> the
>> offsets of the following fields in clnt_addr_t, and that would break
>> other legacy mount code.
>>
>>> It seems at the beginning of each routine saddr is always being
>>> typecast into a struct sockaddr pointer....
>>
>> More likely it is:
>>
>>  struct sockaddr *sap = (struct sockaddr *)&saddr;
>>
>> Which is appropriate and normal.
> More to the point, I see a number of
>
>    struct sockaddr *nfs_saddr = (struct sockaddr *)&nfs_server->saddr;
>
> at the top of routines in the patch set you recently posted. To me it
> seems like it would make more sense if saddr would was a struct  
> sockaddr
> to start with instead of always doing those typecasts... but its truly
> just a nit...

The point of all this is that I wanted to leave probe_bothports()  
legacy callers alone.  I looked into this kind of change, but it would  
have required some extensive modification of legacy code in nfsmount.c  
that is better left alone.

So I created a new API for an IPv6-enabled probe_bothports().  The old  
probe_bothports() is for legacy callers (right now, just  
nfs_call_mount() in nfsmount.c).  It converts the incoming clnt_addr_t  
into pointers to each of its fields.  The new call,  
nfs_probe_bothports(), is for new callers which may want to pass in a  
"struct sockaddr *" that might point to either an IPv4 or IPv6 address.

Both APIs share the same underlying code.  The underlying code uses  
"struct sockaddr *", thus the old legacy interface has to convert from  
clnt_addr_t to "struct sockaddr *".

To me, changing clnt_addr_t would be penny-wise but pound-foolish.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 0/3] AF_INET6 support for probe_bothports()

[Steve Dickson]

Chuck Lever wrote:
> Hi Steve-
> 
> Here's the next step.
> 
> This small series of patches rewires probe_bothports() to support
> AF_INET6 addresses, now that the underlying functions can handle them.
> 
> Since legacy code in other parts of the mount command still use
> probe_bothports() and the clnt_addr_t data type, I've added a new
> function call to do the IPv6 duties.  The old API still exists and
> continues to support only AF_INET, but under the covers it calls the
> new code.
> 
> Again, for the time being, this is used only for the legacy binary
> mount(2) interface.  We will need this for umount later, and that is
> used to support both binary and text-based mounts.
> 
All three patches have been committed... Again sorry for the delay.

That darn flux capacitor keeps breaking all the time
which always seems to get in the way...  ;-)

steved.