Re: [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2021-01-21 at 12:23 -0500, David Wysochanski wrote:
> On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:
> > > Remove duplicated helper functions to parse opaque XDR objects
> > > and place inside the xdr.h file.  Also update comment which
> > > is wrong since lockd is not the only user of xdr_netobj.
> > > 
> > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > > ---
> > >  include/linux/sunrpc/xdr.h          | 33
> > > +++++++++++++++++++++++++++++++--
> > >  net/sunrpc/auth_gss/auth_gss.c      | 29 -----------------------
> > > ----
> > > --
> > >  net/sunrpc/auth_gss/gss_krb5_mech.c | 29 -----------------------
> > > ----
> > > --
> > >  3 files changed, 31 insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/include/linux/sunrpc/xdr.h
> > > b/include/linux/sunrpc/xdr.h
> > > index 19b6dea27367..8ef788ff80b9 100644
> > > --- a/include/linux/sunrpc/xdr.h
> > > +++ b/include/linux/sunrpc/xdr.h
> > > @@ -25,8 +25,7 @@
> > >  #define XDR_QUADLEN(l)         (((l) + 3) >> 2)
> > > 
> > >  /*
> > > - * Generic opaque `network object.' At the kernel level, this
> > > type
> > > - * is used only by lockd.
> > > + * Generic opaque `network object.'
> > >   */
> > >  #define XDR_MAX_NETOBJ         1024
> > >  struct xdr_netobj {
> > > @@ -34,6 +33,36 @@ struct xdr_netobj {
> > >         u8 *                    data;
> > >  };
> > > 
> > > +static inline const void *
> > > +simple_get_bytes(const void *p, const void *end, void *res,
> > > size_t
> > > len)
> > > +{
> > > +       const void *q = (const void *)((const char *)p + len);
> > > +       if (unlikely(q > end || q < p))
> > > +               return ERR_PTR(-EFAULT);
> > > +       memcpy(res, p, len);
> > > +       return q;
> > > +}
> > > +
> > > +static inline const void *
> > > +simple_get_netobj(const void *p, const void *end, struct
> > > xdr_netobj
> > > *dest)
> > > +{
> > > +       const void *q;
> > > +       unsigned int len;
> > > +
> > > +       p = simple_get_bytes(p, end, &len, sizeof(len));
> > > +       if (IS_ERR(p))
> > > +               return p;
> > > +       q = (const void *)((const char *)p + len);
> > > +       if (unlikely(q > end || q < p))
> > > +               return ERR_PTR(-EFAULT);
> > > +       dest->data = kmemdup(p, len, GFP_NOFS);
> > > +       if (unlikely(dest->data == NULL))
> > > +               return ERR_PTR(-ENOMEM);
> > > +       dest->len = len;
> > > +       return q;
> > > +}
> > > +
> > 
> > Hmm... I'm not too keen on having these in sunrpc/xdr.h. For one
> > thing,
> > they do not describe objects that are XDR encoded. Secondly, their
> > naming isn't really consistent with any of the existing conventions
> > for
> > xdr.
> > 
> > Can we please rather just create a new header file that is private
> > in
> > net/sunrpc/auth_gss/ ?
> > 
> 
> Sure.  Do you have any preference for the name?
> internal.h
> auth_gss.h
> something else...
> 

It is unfortunate that we already have an auth_gss.h in
include/linux/sunrpc. How about auth_gss_internal.h instead?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com