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;
MIME-Version: 1.0
References: <168426587118.74246.214357450560967997.stgit@oracle-102.nfsv4bat.org>
 <168426600329.74246.10545150506762914826.stgit@oracle-102.nfsv4bat.org>
In-Reply-To: <168426600329.74246.10545150506762914826.stgit@oracle-102.nfsv4bat.org>
From:   Anna Schumaker <schumaker.anna@gmail.com>
Date:   Mon, 22 May 2023 16:35:34 -0400
Message-ID: <CAFX2Jfk9up-eyLhe7s65E6+vBTjXrATREFoYJVkCBLAT_56o2g@mail.gmail.com>
Subject: Re: [PATCH RFC 04/12] SUNRPC: Refactor rpc_call_null_helper()
To:     Chuck Lever <cel@kernel.org>
Cc:     anna.schumaker@netapp.com, trondmy@hammerspace.com,
        Jeff Layton <jlayton@kernel.org>,
        Chuck Lever <chuck.lever@oracle.com>,
        linux-nfs@vger.kernel.org, kernel-tls-handshake@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Chuck,

On Tue, May 16, 2023 at 3:42=E2=80=AFPM Chuck Lever <cel@kernel.org> wrote:
>
> From: Chuck Lever <chuck.lever@oracle.com>
>
> I'm about to add a use case that does not want RPC_TASK_NULLCREDS.
> Refactor rpc_call_null_helper() so that callers provide NULLCREDS
> when they need it.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/clnt.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 4cdb539b5854..2dca0ae489ec 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2811,8 +2811,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_cl=
nt *clnt,
>                 .rpc_op_cred =3D cred,
>                 .callback_ops =3D ops ?: &rpc_null_ops,
>                 .callback_data =3D data,
> -               .flags =3D flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
> -                        RPC_TASK_NULLCREDS,
> +               .flags =3D flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN,
>         };
>
>         return rpc_run_task(&task_setup_data);
> @@ -2820,7 +2819,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_cl=
nt *clnt,
>
>  struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *c=
red, int flags)
>  {
> -       return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL);
> +       return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NU=
LLCREDS,
> +                                   NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(rpc_call_null);

I think you missed updating rpc_ping() right below this function as
well, I'm unable to mount without the flag. Although I do wonder if it
would be easier to slightly rename rpc_call_null_helper(), and then
create a new rpc_call_null_helper() that appends the
RPC_TASK_NULLCREDS flag. Then we don't need to touch current callers,
and your new use case could call the renamed function.

What do you think?
Anna

>
> @@ -2920,12 +2920,13 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *c=
lnt,
>                 goto success;
>         }
>
> -       task =3D rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC,
> -                       &rpc_cb_add_xprt_call_ops, data);
> +       task =3D rpc_call_null_helper(clnt, xprt, NULL,
> +                                   RPC_TASK_ASYNC | RPC_TASK_NULLCREDS,
> +                                   &rpc_cb_add_xprt_call_ops, data);
>         if (IS_ERR(task))
>                 return PTR_ERR(task);
> -
>         data->xps->xps_nunique_destaddr_xprts++;
> +
>         rpc_put_task(task);
>  success:
>         return 1;
> @@ -2940,7 +2941,8 @@ static int rpc_clnt_add_xprt_helper(struct rpc_clnt=
 *clnt,
>         int status =3D -EADDRINUSE;
>
>         /* Test the connection */
> -       task =3D rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL);
> +       task =3D rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCRED=
S,
> +                                   NULL, NULL);
>         if (IS_ERR(task))
>                 return PTR_ERR(task);
>
>
>