On Tue, 01 Aug 2023, Jeff Layton wrote:
> On Sat, 2023-07-29 at 08:13 +1000, NeilBrown wrote:
> > On Sat, 29 Jul 2023, Lorenzo Bianconi wrote:
> > > Introduce verbose parameter to utility routine in order to reduce output
> > > verbosity. This is a preliminary patch to add rpc_status entry in nfsd
> > > debug filesystem in order to dump pending RPC requests debugging
> > > information.
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > ---
> > > include/linux/sunrpc/svc_xprt.h | 12 ++++++------
> > > net/sunrpc/svc_xprt.c | 2 +-
> > > 2 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> > > index a6b12631db21..1715d4c6bd6b 100644
> > > --- a/include/linux/sunrpc/svc_xprt.h
> > > +++ b/include/linux/sunrpc/svc_xprt.h
> > > @@ -209,21 +209,21 @@ static inline unsigned short svc_xprt_remote_port(const struct svc_xprt *xprt)
> > > }
> > >
> > > static inline char *__svc_print_addr(const struct sockaddr *addr,
> > > - char *buf, const size_t len)
> > > + char *buf, const size_t len,
> > > + bool verbose)
> > > {
> > > const struct sockaddr_in *sin = (const struct sockaddr_in *)addr;
> > > const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)addr;
> > >
> > > switch (addr->sa_family) {
> > > case AF_INET:
> > > - snprintf(buf, len, "%pI4, port=%u", &sin->sin_addr,
> > > - ntohs(sin->sin_port));
> > > + snprintf(buf, len, "%pI4, %s%u", &sin->sin_addr,
> > > + verbose ? "port=" : "", ntohs(sin->sin_port));
> >
> > I would move the "," into the verbose part too.
> > so
> > verbose ? ", port=" : " "
> >
> > Other than that, I like this approach.
> >
>
> If we're separating fields in the main seqfile by ' ', then we probably
> want to use a different delimiter here in the !verbose case. Maybe ':'
> or ',' instead?
Aren't the address and the port two different fields?
Colon is normal for separating host and port, but as IPv6 addresses
contain colons, you would need [IP::V6]:port which is probably isn't
really an improvement.
I would leave address and port as separate fields.
>
> Also, ntohs is going to return a short, so the format should probably be
> using "%hu" for the port.
Probably.
Thanks,
NeilBrown
>
> >
> > > break;
> > >
> > > case AF_INET6:
> > > - snprintf(buf, len, "%pI6, port=%u",
> > > - &sin6->sin6_addr,
> > > - ntohs(sin6->sin6_port));
> > > + snprintf(buf, len, "%pI6, %s%u", &sin6->sin6_addr,
> > > + verbose ? "port=" : "", ntohs(sin6->sin6_port));
> > > break;
> > >
> > > default:
> > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > > index 62c7919ea610..16b794d291a4 100644
> > > --- a/net/sunrpc/svc_xprt.c
> > > +++ b/net/sunrpc/svc_xprt.c
> > > @@ -386,7 +386,7 @@ EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
> > > */
> > > char *svc_print_addr(struct svc_rqst *rqstp, char *buf, size_t len)
> > > {
> > > - return __svc_print_addr(svc_addr(rqstp), buf, len);
> > > + return __svc_print_addr(svc_addr(rqstp), buf, len, true);
> > > }
> > > EXPORT_SYMBOL_GPL(svc_print_addr);
> > >
> > > --
> > > 2.41.0
> > >
> > >
> >
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
