Re: [PATCH v4 1/8] NFSD fill-in netloc4 structure

[Olga Kornievskaia <olga.kornievskaia@gmail.com>]

On Mon, Jul 22, 2019 at 3:59 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Wed, Jul 17, 2019 at 5:13 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Mon, Jul 08, 2019 at 03:23:45PM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > nfs.4 defines nfs42_netaddr structure that represents netloc4.
> > >
> > > Populate needed fields from the sockaddr structure.
> > >
> > > This will be used by flexfiles and 4.2 inter copy
> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  fs/nfsd/nfsd.h | 32 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > > index 24187b5..8f4fc50 100644
> > > --- a/fs/nfsd/nfsd.h
> > > +++ b/fs/nfsd/nfsd.h
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/sunrpc/svc.h>
> > >  #include <linux/sunrpc/svc_xprt.h>
> > >  #include <linux/sunrpc/msg_prot.h>
> > > +#include <linux/sunrpc/addr.h>
> > >
> > >  #include <uapi/linux/nfsd/debug.h>
> > >
> > > @@ -375,6 +376,37 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> > >
> > >  extern const u32 nfsd_suppattrs[3][3];
> > >
> > > +static inline u32 nfsd4_set_netaddr(struct sockaddr *addr,
> > > +                                 struct nfs42_netaddr *netaddr)
> > > +{
> > > +     struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> > > +     struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
> > > +     unsigned int port;
> > > +     size_t ret_addr, ret_port;
> > > +
> > > +     switch (addr->sa_family) {
> > > +     case AF_INET:
> > > +             port = ntohs(sin->sin_port);
> > > +             sprintf(netaddr->netid, "tcp");
> > > +             netaddr->netid_len = 3;
> > > +             break;
> > > +     case AF_INET6:
> > > +             port = ntohs(sin6->sin6_port);
> > > +             sprintf(netaddr->netid, "tcp6");
> > > +             netaddr->netid_len = 4;
> > > +             break;
> > > +     default:
> > > +             return nfserr_inval;
> > > +     }
> > > +     ret_addr = rpc_ntop(addr, netaddr->addr, sizeof(netaddr->addr));
> > > +     ret_port = snprintf(netaddr->addr + ret_addr,
> > > +                         RPCBIND_MAXUADDRLEN + 1 - ret_addr,
> > > +                         ".%u.%u", port >> 8, port & 0xff);
> > > +     WARN_ON(ret_port >= RPCBIND_MAXUADDRLEN + 1 - ret_addr);
> > > +     netaddr->addr_len = ret_addr + ret_port;
> > > +     return 0;
> > > +}
> >
> > Kinda surprised we don't already do something like this elsewhere, but I
> > don't see anything exactly the same.  Might be possible to put this in
> > net/sunrpc/addr.c and share some code with rpc_sockaddr2uaddr?  I'll
> > leave it to you whether that looks worth it.
>
> I'll investigate how to move this into the sunrpc. Client also
> populates nfs42_netaddr structure but it's slightly different. I need
> to go back and see if what's shareable.

Ok I'd like argue for the code to stay as is because
1. can't move the whole function into addr.c because it created a data
structure (nfs42_netaddr) that rpc knows nothing about
2. While the nfs42_netaddr->addr is the output of the rpc_sock2uaddr()
but we still need the switch to populate the netid . Also since
rpc_sock2uaddr returns memory than the nfs42_netaddr data structure
needs to change to store pointers (and that's shared with the client).
Thus client and server would need to add other code to free the
created netaddr.
3. this function as is can be used by the flexfile layout as well
(they also decided not to share code with rpc_sockaddr2uaddr but use
same content). that function also doesn't want the memory to be
allocated.

Maybe I'm wrong about all of it and it all needs to be re-written to
take dynamic memory. But to use as is I don't want to call it and then
memcpy into existing static buffers and freeing what
rpc_sockaddr2uaddr has allocated.



>
> >
> > --b.
> >
> > > +
> > >  static inline bool bmval_is_subset(const u32 *bm1, const u32 *bm2)
> > >  {
> > >       return !((bm1[0] & ~bm2[0]) ||
> > > --
> > > 1.8.3.1