| Message ID | 2b32d41cf6a502918a685447cd749c4b1cb0d16d.1635685588.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xprtrdma: Fix a maybe-uninitialized compiler warning | expand |
> 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; > @@ -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 > -- Chuck Lever
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. > > @@ -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.
> 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. >>> @@ -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
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 > > >
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; @@ -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;
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> --- net/sunrpc/xprtrdma/frwr_ops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)