Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Lorenzo Bianconi" <lorenzo@kernel.org>
Cc:     linux-nfs@vger.kernel.org, lorenzo.bianconi@redhat.com,
        chuck.lever@oracle.com, jlayton@kernel.org
Subject: Re: [PATCH v4 1/2] SUNRPC: add verbose parameter to __svc_print_addr()
In-reply-to: <5b0eff4e3ef9bf9621f5095189933f60def40f0d.1690569488.git.lorenzo@kernel.org>
References: <cover.1690569488.git.lorenzo@kernel.org>,
 <5b0eff4e3ef9bf9621f5095189933f60def40f0d.1690569488.git.lorenzo@kernel.org>
Date:   Sat, 29 Jul 2023 08:13:13 +1000
Message-id: <169058239308.32308.14184895022271604035@noble.neil.brown.name>
Precedence: bulk

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.
>=20
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  include/linux/sunrpc/svc_xprt.h | 12 ++++++------
>  net/sunrpc/svc_xprt.c           |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
>=20
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xpr=
t.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(con=
st struct svc_xprt *xprt)
>  }
> =20
>  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 =3D (const struct sockaddr_in *)addr;
>  	const struct sockaddr_in6 *sin6 =3D (const struct sockaddr_in6 *)addr;
> =20
>  	switch (addr->sa_family) {
>  	case AF_INET:
> -		snprintf(buf, len, "%pI4, port=3D%u", &sin->sin_addr,
> -			ntohs(sin->sin_port));
> +		snprintf(buf, len, "%pI4, %s%u", &sin->sin_addr,
> +			 verbose ? "port=3D" : "", ntohs(sin->sin_port));

I would move the "," into the verbose part too.
so
   verbose ? ", port=3D" : " "

Other than that, I like this approach.

NeilBrown

>  		break;
> =20
>  	case AF_INET6:
> -		snprintf(buf, len, "%pI6, port=3D%u",
> -			 &sin6->sin6_addr,
> -			ntohs(sin6->sin6_port));
> +		snprintf(buf, len, "%pI6, %s%u", &sin6->sin6_addr,
> +			 verbose ? "port=3D" : "", ntohs(sin6->sin6_port));
>  		break;
> =20
>  	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);
> =20
> --=20
> 2.41.0
>=20
>=20