[PATCH] RFC: backwards compatibility for TI-RPC

[PATCH] RFC: backwards compatibility for TI-RPC

[Chuck Lever]

Hi Steve, Olaf -

Normally I don't post a single patch with a cover letter, but this one
needs some 'splainin'.

I've tracked down a problem where rpc.statd in the latest nfs-utils
git repo breaks.  Even though rpc.statd hasn't changed yet, it is
now calling legacy RPC functions in TI-RPC rather than the original
Linux RPC implementation in glibc.  There appear to be some ABI
incompatibilities between the two.

I'd like to solicit comments on this approach of addressing this
problem.  And, more generally, also address the question of whether
we want to sponsor a more extensive effort to review our recently
adopted TI-RPC implementation to ensure that we have good ABI
compatibility with glibc's RPC implementation.  Should we consider
a strategy of merging these two implementations over time?

Talk amongst yourselves...

---

Chuck Lever (1):
      backwards compatibility: fix order of fields in TI-RPC's svc_req


 tirpc/rpc/svc.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

-- 
Chuck Lever <chuck.lever@oracle.com>

[PATCH] backwards compatibility: fix order of fields in TI-RPC's svc_req

[Chuck Lever]

Preserve ABI compatibility between glibc's RPC implementation and
the legacy RPC implementation in libtirpc by moving the rq_xprt
field in the TI-RPC version of the svc_req struct so it is
backwards compatible with the legacy version of this structure.

Linux's legacy svc_req struct, from /usr/include/rpc/svc.h, looks
like this:

struct svc_req {
  rpcprog_t rq_prog;		/* service program number */
  rpcvers_t rq_vers;		/* service protocol version */
  rpcproc_t rq_proc;		/* the desired procedure */
  struct opaque_auth rq_cred;	/* raw creds from the wire */
  caddr_t rq_clntcred;		/* read only cooked cred */
  SVCXPRT *rq_xprt;		/* associated transport */
};

The new TI-RPC svc_req struct, from /usr/include/tirpc/rpc/svc.h,
looks like this:

struct svc_req {
  u_int32_t rq_prog;		/* service program number */
  u_int32_t rq_vers;		/* service protocol version */
  u_int32_t rq_proc;		/* the desired procedure */
  struct opaque_auth rq_cred;	/* raw creds from the wire */
  void *rq_clntcred;		/* read only cooked cred */
  caddr_t rq_clntname;		/* read only client name */
  caddr_t rq_svcname;		/* read only cooked service cred */
  SVCXPRT *rq_xprt;		/* associated transport */
};

Note the extra fields rq_clntname and rq_svcname.  These are used for
TI-RPC's RPCSEC GSS flavor support.

This issue came to light because rpc.statd still uses only legacy RPC
calls, and thus includes /usr/include/rpc/svc.h.  However, other parts
of nfs-utils now link with TI-RPC, so the legacy RPC functions in
libtirpc are used in favor of glibc's RPC functions.  The libtirpc svc
functions use the new svc_req struct, but rpc.statd uses the old
svc_req struct.

Since the svc_req fields were different, rpc.statd broke after recent
IPv6-related changes, even though I hadn't made any changes to it.
Note that rpc.mountd also references the rq_xprt field, so it has the
same issue.

In most operating systems, there is only one rpc/svc.h and one version
of svc_req so this is not a problem.  We should audit all of the
structures and functions under /usr/include/rpc and
/usr/include/tirpc/rpc to ensure we have a reasonable level of
backwards compatibility until such a time it is decided to merge these
implementations or get rid of RPC support in glibc.

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

 tirpc/rpc/svc.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tirpc/rpc/svc.h b/tirpc/rpc/svc.h
index ec5914f..ea2985d 100644
--- a/tirpc/rpc/svc.h
+++ b/tirpc/rpc/svc.h
@@ -131,14 +131,17 @@ typedef struct __rpc_svcxprt {
  * Service request
  */
 struct svc_req {
+	/* ORDER: compatibility with legacy RPC */
 	u_int32_t	rq_prog;	/* service program number */
 	u_int32_t	rq_vers;	/* service protocol version */
 	u_int32_t	rq_proc;	/* the desired procedure */
 	struct opaque_auth rq_cred;	/* raw creds from the wire */
 	void		*rq_clntcred;	/* read only cooked cred */
+	SVCXPRT		*rq_xprt;	/* associated transport */
+
+	/* New with TI-RPC */
 	caddr_t		rq_clntname;	/* read only client name */
 	caddr_t		rq_svcname;	/* read only cooked service cred */
-	SVCXPRT		*rq_xprt;	/* associated transport */
 };
 
 /*

Re: [PATCH] backwards compatibility: fix order of fields in TI-RPC's svc_req

[J. Bruce Fields]

On Fri, Dec 19, 2008 at 10:31:33AM -0500, Chuck Lever wrote:
> Preserve ABI compatibility between glibc's RPC implementation and
> the legacy RPC implementation in libtirpc by moving the rq_xprt
> field in the TI-RPC version of the svc_req struct so it is
> backwards compatible with the legacy version of this structure.
> 
> Linux's legacy svc_req struct, from /usr/include/rpc/svc.h, looks
> like this:
> 
> struct svc_req {
>   rpcprog_t rq_prog;		/* service program number */
>   rpcvers_t rq_vers;		/* service protocol version */
>   rpcproc_t rq_proc;		/* the desired procedure */
>   struct opaque_auth rq_cred;	/* raw creds from the wire */
>   caddr_t rq_clntcred;		/* read only cooked cred */
>   SVCXPRT *rq_xprt;		/* associated transport */
> };
> 
> The new TI-RPC svc_req struct, from /usr/include/tirpc/rpc/svc.h,
> looks like this:
> 
> struct svc_req {
>   u_int32_t rq_prog;		/* service program number */
>   u_int32_t rq_vers;		/* service protocol version */
>   u_int32_t rq_proc;		/* the desired procedure */
>   struct opaque_auth rq_cred;	/* raw creds from the wire */
>   void *rq_clntcred;		/* read only cooked cred */
>   caddr_t rq_clntname;		/* read only client name */
>   caddr_t rq_svcname;		/* read only cooked service cred */
>   SVCXPRT *rq_xprt;		/* associated transport */
> };
> 
> Note the extra fields rq_clntname and rq_svcname.  These are used for
> TI-RPC's RPCSEC GSS flavor support.
> 
> This issue came to light because rpc.statd still uses only legacy RPC
> calls, and thus includes /usr/include/rpc/svc.h.  However, other parts
> of nfs-utils now link with TI-RPC, so the legacy RPC functions in
> libtirpc are used in favor of glibc's RPC functions.  The libtirpc svc
> functions use the new svc_req struct, but rpc.statd uses the old
> svc_req struct.
> 
> Since the svc_req fields were different, rpc.statd broke after recent
> IPv6-related changes, even though I hadn't made any changes to it.
> Note that rpc.mountd also references the rq_xprt field, so it has the
> same issue.
> 
> In most operating systems, there is only one rpc/svc.h and one version
> of svc_req so this is not a problem.  We should audit all of the
> structures and functions under /usr/include/rpc and
> /usr/include/tirpc/rpc to ensure we have a reasonable level of
> backwards compatibility until such a time it is decided to merge these
> implementations or get rid of RPC support in glibc.

I think the glibc people don't want to update their RPC support--so it
should stay there as a legacy thing for backwards compatibility, but
eventually applications should be using something else.

As long as that "something else" supports both ipv6 and rpcsec_gss, I'm
happy....

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  tirpc/rpc/svc.h |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/tirpc/rpc/svc.h b/tirpc/rpc/svc.h
> index ec5914f..ea2985d 100644
> --- a/tirpc/rpc/svc.h
> +++ b/tirpc/rpc/svc.h
> @@ -131,14 +131,17 @@ typedef struct __rpc_svcxprt {
>   * Service request
>   */
>  struct svc_req {
> +	/* ORDER: compatibility with legacy RPC */
>  	u_int32_t	rq_prog;	/* service program number */
>  	u_int32_t	rq_vers;	/* service protocol version */
>  	u_int32_t	rq_proc;	/* the desired procedure */
>  	struct opaque_auth rq_cred;	/* raw creds from the wire */
>  	void		*rq_clntcred;	/* read only cooked cred */
> +	SVCXPRT		*rq_xprt;	/* associated transport */
> +
> +	/* New with TI-RPC */
>  	caddr_t		rq_clntname;	/* read only client name */
>  	caddr_t		rq_svcname;	/* read only cooked service cred */
> -	SVCXPRT		*rq_xprt;	/* associated transport */
>  };
>  
>  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] backwards compatibility: fix order of fields in TI-RPC's svc_req

[Steve Dickson]

Chuck Lever wrote:
> Preserve ABI compatibility between glibc's RPC implementation and
> the legacy RPC implementation in libtirpc by moving the rq_xprt
> field in the TI-RPC version of the svc_req struct so it is
> backwards compatible with the legacy version of this structure.
> 
> Linux's legacy svc_req struct, from /usr/include/rpc/svc.h, looks
> like this:
> 
> struct svc_req {
>   rpcprog_t rq_prog;		/* service program number */
>   rpcvers_t rq_vers;		/* service protocol version */
>   rpcproc_t rq_proc;		/* the desired procedure */
>   struct opaque_auth rq_cred;	/* raw creds from the wire */
>   caddr_t rq_clntcred;		/* read only cooked cred */
>   SVCXPRT *rq_xprt;		/* associated transport */
> };
> 
> The new TI-RPC svc_req struct, from /usr/include/tirpc/rpc/svc.h,
> looks like this:
> 
> struct svc_req {
>   u_int32_t rq_prog;		/* service program number */
>   u_int32_t rq_vers;		/* service protocol version */
>   u_int32_t rq_proc;		/* the desired procedure */
>   struct opaque_auth rq_cred;	/* raw creds from the wire */
>   void *rq_clntcred;		/* read only cooked cred */
>   caddr_t rq_clntname;		/* read only client name */
>   caddr_t rq_svcname;		/* read only cooked service cred */
>   SVCXPRT *rq_xprt;		/* associated transport */
> };
> 
> Note the extra fields rq_clntname and rq_svcname.  These are used for
> TI-RPC's RPCSEC GSS flavor support.
> 
> This issue came to light because rpc.statd still uses only legacy RPC
> calls, and thus includes /usr/include/rpc/svc.h.  However, other parts
> of nfs-utils now link with TI-RPC, so the legacy RPC functions in
> libtirpc are used in favor of glibc's RPC functions.  The libtirpc svc
> functions use the new svc_req struct, but rpc.statd uses the old
> svc_req struct.
> 
> Since the svc_req fields were different, rpc.statd broke after recent
> IPv6-related changes, even though I hadn't made any changes to it.
> Note that rpc.mountd also references the rq_xprt field, so it has the
> same issue.
> 
> In most operating systems, there is only one rpc/svc.h and one version
> of svc_req so this is not a problem.  We should audit all of the
> structures and functions under /usr/include/rpc and
> /usr/include/tirpc/rpc to ensure we have a reasonable level of
> backwards compatibility until such a time it is decided to merge these
> implementations or get rid of RPC support in glibc.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
Committed....

steved.