Re: [PATCH] xprtrdma: Fix a maybe-uninitialized compiler warning

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2021-11-02 at 16:50 +0000, Chuck Lever III wrote:
> 
> 
> > On Nov 2, 2021, at 12:43 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Sun, 2021-10-31 at 15:04 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Oct 31, 2021, at 9:08 AM, Benjamin Coddington
> > > > <bcodding@redhat.com> wrote:
> > > > 
> > > > This minor fix-up keeps GCC from complaining that "last' may be
> > > > used
> > > > uninitialized", which breaks some build workflows that have
> > > > been
> > > > running
> > > > with all warnings treated as errors.
> > > > 
> > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > 
> > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > 
> > > > ---
> > > > net/sunrpc/xprtrdma/frwr_ops.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/sunrpc/xprtrdma/frwr_ops.c
> > > > b/net/sunrpc/xprtrdma/frwr_ops.c
> > > > index f700b34a5bfd..de813fa07daa 100644
> > > > --- a/net/sunrpc/xprtrdma/frwr_ops.c
> > > > +++ b/net/sunrpc/xprtrdma/frwr_ops.c
> > > > @@ -503,7 +503,7 @@ static void frwr_wc_localinv_wake(struct
> > > > ib_cq
> > > > *cq, struct ib_wc *wc)
> > > >  */
> > > > void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct
> > > > rpcrdma_req
> > > > *req)
> > > > {
> > > > -       struct ib_send_wr *first, **prev, *last;
> > > > +       struct ib_send_wr *first, **prev, *last = NULL;
> > > >         struct rpcrdma_ep *ep = r_xprt->rx_ep;
> > > >         const struct ib_send_wr *bad_wr;
> > > >         struct rpcrdma_mr *mr;
> > 
> > Err... Definitely not sufficient.
> > 
> > gcc is absolutely correct to complain here, because if req-
> > > rl_registered is empty, then the whole rest of the function after
> > > that
> > while() loop is invalid.
> 
> The callers ensure rl_registered is not empty.
> 
> This used to be preferred: don't add code to check conditions
> that are known to be true. If that policy is different now,
> then yes, this code will have to be restructured.
> 

If that's the case, then please change those two "while() {}" blocks
into "do { } while();" so that we avoid the apparently unnecessary
first check for whether the list is empty. That would be the real fix
here, and one that clearly documents the expectation.

> 
> > > > @@ -608,7 +608,7 @@ static void frwr_wc_localinv_done(struct
> > > > ib_cq
> > > > *cq, struct ib_wc *wc)
> > > >  */
> > > > void frwr_unmap_async(struct rpcrdma_xprt *r_xprt, struct
> > > > rpcrdma_req *req)
> > > > {
> > > > -       struct ib_send_wr *first, *last, **prev;
> > > > +       struct ib_send_wr *first, *last = NULL, **prev;
> > > >         struct rpcrdma_ep *ep = r_xprt->rx_ep;
> > > >         struct rpcrdma_mr *mr;
> > > >         int rc;
> > > > -- 
> > > > 2.31.1
> > > > 
> > 
> > Ditto.
> > 
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
​
www.hammer.space