Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200818040317.5926-1-brookebasile@gmail.com> <20200818052957.GA2253299@ubuntu-n2-xlarge-x86>
In-Reply-To: <20200818052957.GA2253299@ubuntu-n2-xlarge-x86>
From:   Jinpu Wang <jinpu.wang@cloud.ionos.com>
Date:   Tue, 18 Aug 2020 08:42:06 +0200
Message-ID: <CAMGffE=2m7XOdKS9xG1bNY7MYR2x6AgQm9YZFHm6D4biO860sw@mail.gmail.com>
Subject: Re: [PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning
To:     Nathan Chancellor <natechancellor@gmail.com>
Cc:     Brooke Basile <brookebasile@gmail.com>,
        Danil Kipnis <danil.kipnis@cloud.ionos.com>,
        Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 18, 2020 at 7:30 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Aug 18, 2020 at 12:03:18AM -0400, Brooke Basile wrote:
> > Clang warns:
> >       drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
> >       uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> >               if (IS_ERR(bio)) {
> >               ^~~~~~~~~~~
> >       drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
> >               return err;
> >               ^~~
> >       drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
> >       to silence this warning
> >               int err;
> >                       ^
> >                       = 0
> >
> > Silence this by replacing `err` with `ret`, returning ret = 0 upon
> > success.
> >
> > Signed-off-by: Brooke Basile <brookebasile@gmail.com>
> > ---
> >  drivers/block/rnbd/rnbd-srv.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> > index 0fb94843a495..f515d1a048a9 100644
> > --- a/drivers/block/rnbd/rnbd-srv.c
> > +++ b/drivers/block/rnbd/rnbd-srv.c
> > @@ -123,10 +123,10 @@ static int process_rdma(struct rtrs_srv *sess,
> >       struct rnbd_io_private *priv;
> >       struct rnbd_srv_sess_dev *sess_dev;
> >       u32 dev_id;
> > -     int err;
> >       struct rnbd_dev_blk_io *io;
> >       struct bio *bio;
> >       short prio;
> > +     int ret = 0;
> >
> >       priv = kmalloc(sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -138,7 +138,7 @@ static int process_rdma(struct rtrs_srv *sess,
> >       if (IS_ERR(sess_dev)) {
> >               pr_err_ratelimited("Got I/O request on session %s for unknown device id %d\n",
> >                                  srv_sess->sessname, dev_id);
> > -             err = -ENOTCONN;
> > +             ret = -ENOTCONN;
> >               goto err;
> >       }
> >
> > @@ -168,13 +168,13 @@ static int process_rdma(struct rtrs_srv *sess,
> >
> >       submit_bio(bio);
> >
> > -     return 0;
> > +     return ret;
> >
> >  sess_dev_put:
> >       rnbd_put_sess_dev(sess_dev);
> >  err:
> >       kfree(priv);
> > -     return err;
> > +     return ret;
> >  }
> >
> >  static void destroy_device(struct rnbd_srv_dev *dev)
> > --
> > 2.28.0
> >
>
> I don't think this is a proper fix since the root cause of the warning
> appears to be that we are ignoring the return value of
> rnbd_bio_map_kern. Should we not set err to that value like this
> (completely untested)?
>
> Cheers,
> Nathan
Thanks Nathan, thanks Brooke,

I agree with Nathan, the problem is we forgot to set the err before
"goto sess_dev_put".
Nathan's patch is simpler, less code of change.
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 0fb94843a495..1b71cb2a885d 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
>         /* Generate bio with pages pointing to the rdma buffer */
>         bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
>         if (IS_ERR(bio)) {
> -               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
> +               err = PTR_ERR(bio);
> +               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
>                 goto sess_dev_put;
>         }
>
Nathan, could you send a formal patch?

Thanks