| Message ID | 20210110215726.861269-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/rnbd-clt: improve find_or_create_sess() return check | expand |
On Sun, Jan 10, 2021 at 01:57:26PM -0800, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > clang static analysis reports this problem > > rnbd-clt.c:1212:11: warning: Branch condition evaluates to a > garbage value > else if (!first) > ^~~~~~ Ah, is it complaining that the 'if (IS_ERR(sess)) {' section in find_or_create_sess() does not initialize first even though that will be caught by the 'if (sess == ERR_PTR(-ENOMEM))' in find_and_get_or_create_sess() because alloc_sess() only returns an -ENOMEM error pointer? > This is triggered in the find_and_get_or_create_sess() call > because the variable first is not initialized and the > earlier check is specifically for > > if (sess == ERR_PTR(-ENOMEM)) > > This is false positive. > > But the if-check can be reduced by initializing first to > false and then returning if the call to find_or_creat_sess() > does not set it to true. When it remains false, either > sess will be valid or not. The not case is caught by > find_and_get_or_create_sess()'s caller rnbd_clt_map_device() > > sess = find_and_get_or_create_sess(...); > if (IS_ERR(sess)) > return ERR_CAST(sess); > > Since find_and_get_or_create_sess() initializes first to false > setting it in find_or_create_sess() is not needed. > > Signed-off-by: Tom Rix <trix@redhat.com> Every maintainer has their preference for where and how stuff gets initialized but this makes sense to me. I am not sure the commit above find_or_create_sess() is needed but again, personal preference. Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/block/rnbd/rnbd-clt.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c > index 96e3f9fe8241..251f747cf10d 100644 > --- a/drivers/block/rnbd/rnbd-clt.c > +++ b/drivers/block/rnbd/rnbd-clt.c > @@ -919,6 +919,7 @@ static struct rnbd_clt_session *__find_and_get_sess(const char *sessname) > return NULL; > } > > +/* caller is responsible for initializing 'first' to false */ > static struct > rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) > { > @@ -934,8 +935,7 @@ rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) > } > list_add(&sess->list, &sess_list); > *first = true; > - } else > - *first = false; > + } > mutex_unlock(&sess_lock); > > return sess; > @@ -1203,13 +1203,11 @@ find_and_get_or_create_sess(const char *sessname, > struct rnbd_clt_session *sess; > struct rtrs_attrs attrs; > int err; > - bool first; > + bool first = false; > struct rtrs_clt_ops rtrs_ops; > > sess = find_or_create_sess(sessname, &first); > - if (sess == ERR_PTR(-ENOMEM)) > - return ERR_PTR(-ENOMEM); > - else if (!first) > + if (!first) > return sess; > > if (!path_cnt) { > -- > 2.27.0 >
On Sun, Jan 10, 2021 at 10:58 PM <trix@redhat.com> wrote: > > From: Tom Rix <trix@redhat.com> > > clang static analysis reports this problem > > rnbd-clt.c:1212:11: warning: Branch condition evaluates to a > garbage value > else if (!first) > ^~~~~~ > > This is triggered in the find_and_get_or_create_sess() call > because the variable first is not initialized and the > earlier check is specifically for > > if (sess == ERR_PTR(-ENOMEM)) > > This is false positive. > > But the if-check can be reduced by initializing first to > false and then returning if the call to find_or_creat_sess() > does not set it to true. When it remains false, either > sess will be valid or not. The not case is caught by > find_and_get_or_create_sess()'s caller rnbd_clt_map_device() > > sess = find_and_get_or_create_sess(...); > if (IS_ERR(sess)) > return ERR_CAST(sess); > > Since find_and_get_or_create_sess() initializes first to false > setting it in find_or_create_sess() is not needed. > > Signed-off-by: Tom Rix <trix@redhat.com> Less LOC is better :) Acked-by: Jack Wang <jinpu.wang@cloud.ionos.com> Thanks Tom and Nathan! > --- > drivers/block/rnbd/rnbd-clt.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c > index 96e3f9fe8241..251f747cf10d 100644 > --- a/drivers/block/rnbd/rnbd-clt.c > +++ b/drivers/block/rnbd/rnbd-clt.c > @@ -919,6 +919,7 @@ static struct rnbd_clt_session *__find_and_get_sess(const char *sessname) > return NULL; > } > > +/* caller is responsible for initializing 'first' to false */ > static struct > rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) > { > @@ -934,8 +935,7 @@ rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) > } > list_add(&sess->list, &sess_list); > *first = true; > - } else > - *first = false; > + } > mutex_unlock(&sess_lock); > > return sess; > @@ -1203,13 +1203,11 @@ find_and_get_or_create_sess(const char *sessname, > struct rnbd_clt_session *sess; > struct rtrs_attrs attrs; > int err; > - bool first; > + bool first = false; > struct rtrs_clt_ops rtrs_ops; > > sess = find_or_create_sess(sessname, &first); > - if (sess == ERR_PTR(-ENOMEM)) > - return ERR_PTR(-ENOMEM); > - else if (!first) > + if (!first) > return sess; > > if (!path_cnt) { > -- > 2.27.0 >
On 1/10/21 9:14 PM, Nathan Chancellor wrote: > On Sun, Jan 10, 2021 at 01:57:26PM -0800, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> clang static analysis reports this problem >> >> rnbd-clt.c:1212:11: warning: Branch condition evaluates to a >> garbage value >> else if (!first) >> ^~~~~~ > Ah, is it complaining that the 'if (IS_ERR(sess)) {' section in > find_or_create_sess() does not initialize first even though that will be > caught by the 'if (sess == ERR_PTR(-ENOMEM))' in > find_and_get_or_create_sess() because alloc_sess() only returns an > -ENOMEM error pointer? Reviewing the code, failure looks like it returns only -ENOMEM. So the check is correct but brittle. > >> This is triggered in the find_and_get_or_create_sess() call >> because the variable first is not initialized and the >> earlier check is specifically for >> >> if (sess == ERR_PTR(-ENOMEM)) >> >> This is false positive. >> >> But the if-check can be reduced by initializing first to >> false and then returning if the call to find_or_creat_sess() >> does not set it to true. When it remains false, either >> sess will be valid or not. The not case is caught by >> find_and_get_or_create_sess()'s caller rnbd_clt_map_device() >> >> sess = find_and_get_or_create_sess(...); >> if (IS_ERR(sess)) >> return ERR_CAST(sess); >> >> Since find_and_get_or_create_sess() initializes first to false >> setting it in find_or_create_sess() is not needed. >> >> Signed-off-by: Tom Rix <trix@redhat.com> > Every maintainer has their preference for where and how stuff gets > initialized but this makes sense to me. I am not sure the commit above > find_or_create_sess() is needed but again, personal preference. Mostly this removes two unneeded branches at the cost of initializing a variable. Secondary, the static analysis complaint is resolved. Tom > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > >> --- >> drivers/block/rnbd/rnbd-clt.c | 10 ++++------ >> 1 file changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c >> index 96e3f9fe8241..251f747cf10d 100644 >> --- a/drivers/block/rnbd/rnbd-clt.c >> +++ b/drivers/block/rnbd/rnbd-clt.c >> @@ -919,6 +919,7 @@ static struct rnbd_clt_session *__find_and_get_sess(const char *sessname) >> return NULL; >> } >> >> +/* caller is responsible for initializing 'first' to false */ >> static struct >> rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) >> { >> @@ -934,8 +935,7 @@ rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) >> } >> list_add(&sess->list, &sess_list); >> *first = true; >> - } else >> - *first = false; >> + } >> mutex_unlock(&sess_lock); >> >> return sess; >> @@ -1203,13 +1203,11 @@ find_and_get_or_create_sess(const char *sessname, >> struct rnbd_clt_session *sess; >> struct rtrs_attrs attrs; >> int err; >> - bool first; >> + bool first = false; >> struct rtrs_clt_ops rtrs_ops; >> >> sess = find_or_create_sess(sessname, &first); >> - if (sess == ERR_PTR(-ENOMEM)) >> - return ERR_PTR(-ENOMEM); >> - else if (!first) >> + if (!first) >> return sess; >> >> if (!path_cnt) { >> -- >> 2.27.0 >>
diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index 96e3f9fe8241..251f747cf10d 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -919,6 +919,7 @@ static struct rnbd_clt_session *__find_and_get_sess(const char *sessname) return NULL; } +/* caller is responsible for initializing 'first' to false */ static struct rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) { @@ -934,8 +935,7 @@ rnbd_clt_session *find_or_create_sess(const char *sessname, bool *first) } list_add(&sess->list, &sess_list); *first = true; - } else - *first = false; + } mutex_unlock(&sess_lock); return sess; @@ -1203,13 +1203,11 @@ find_and_get_or_create_sess(const char *sessname, struct rnbd_clt_session *sess; struct rtrs_attrs attrs; int err; - bool first; + bool first = false; struct rtrs_clt_ops rtrs_ops; sess = find_or_create_sess(sessname, &first); - if (sess == ERR_PTR(-ENOMEM)) - return ERR_PTR(-ENOMEM); - else if (!first) + if (!first) return sess; if (!path_cnt) {