| Message ID | 1630489412-31008-1-git-send-email-wangyunjian@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] bond: Fix return value check in bond_member_set_may_enable() | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 01/09/2021 10:43, Yunjian Wang wrote: For title, how about: "bond: Check for NULL member in bond_member_set_enable()." > The function bond_member_lookup() could return NULL, the return value > need to be checked. > > Addresses-Coverity: ("Dereference null return value") This is not a typical tag in OVS. If if it is from a private coverity run, you can just mention it was found by coverity. If it is public you can use the the "Reported-at: <link to coverity issue>". Fix itself looks good, thanks. With commit msg changes, Acked-by: Kevin Traynor <ktraynor@redhat.com> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > ofproto/bond.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index a4116588f..2dcfeda71 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -672,8 +672,13 @@ out: > void > bond_member_set_may_enable(struct bond *bond, void *member_, bool may_enable) > { > + struct bond_member *member; > + > ovs_rwlock_wrlock(&rwlock); > - bond_member_lookup(bond, member_)->may_enable = may_enable; > + member = bond_member_lookup(bond, member_); > + if (member) { > + member->may_enable = may_enable; > + } > ovs_rwlock_unlock(&rwlock); > } > >
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Wednesday, September 1, 2021 8:28 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org; > i.maximets@ovn.org > Cc: dingxiaoxiong <dingxiaoxiong@huawei.com> > Subject: Re: [ovs-dev] [PATCH] bond: Fix return value check in > bond_member_set_may_enable() > > On 01/09/2021 10:43, Yunjian Wang wrote: > > For title, how about: > > "bond: Check for NULL member in bond_member_set_enable()." > > > The function bond_member_lookup() could return NULL, the return value > > need to be checked. > > > > Addresses-Coverity: ("Dereference null return value") > > This is not a typical tag in OVS. If if it is from a private coverity run, you can just > mention it was found by coverity. If it is public you can use the the "Reported-at: > <link to coverity issue>". > > Fix itself looks good, thanks. With commit msg changes, > Acked-by: Kevin Traynor <ktraynor@redhat.com> Thanks for the suggestion. V2 has been sent. http://patchwork.ozlabs.org/project/openvswitch/patch/1630579661-34824-1-git-send-email-wangyunjian@huawei.com/ > > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > --- > > ofproto/bond.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/bond.c b/ofproto/bond.c index > > a4116588f..2dcfeda71 100644 > > --- a/ofproto/bond.c > > +++ b/ofproto/bond.c > > @@ -672,8 +672,13 @@ out: > > void > > bond_member_set_may_enable(struct bond *bond, void *member_, bool > > may_enable) { > > + struct bond_member *member; > > + > > ovs_rwlock_wrlock(&rwlock); > > - bond_member_lookup(bond, member_)->may_enable = may_enable; > > + member = bond_member_lookup(bond, member_); > > + if (member) { > > + member->may_enable = may_enable; > > + } > > ovs_rwlock_unlock(&rwlock); > > } > > > >
On 02/09/2021 12:00, wangyunjian wrote: >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Wednesday, September 1, 2021 8:28 PM >> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org; >> i.maximets@ovn.org >> Cc: dingxiaoxiong <dingxiaoxiong@huawei.com> >> Subject: Re: [ovs-dev] [PATCH] bond: Fix return value check in >> bond_member_set_may_enable() >> >> On 01/09/2021 10:43, Yunjian Wang wrote: >> >> For title, how about: >> >> "bond: Check for NULL member in bond_member_set_enable()." >> >>> The function bond_member_lookup() could return NULL, the return value >>> need to be checked. >>> >>> Addresses-Coverity: ("Dereference null return value") >> >> This is not a typical tag in OVS. If if it is from a private coverity run, you can just >> mention it was found by coverity. If it is public you can use the the "Reported-at: >> <link to coverity issue>". >> >> Fix itself looks good, thanks. With commit msg changes, >> Acked-by: Kevin Traynor <ktraynor@redhat.com> > > Thanks for the suggestion. V2 has been sent. > http://patchwork.ozlabs.org/project/openvswitch/patch/1630579661-34824-1-git-send-email-wangyunjian@huawei.com/ > Thanks Yunjian, I'll take a look. >> >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >>> --- >>> ofproto/bond.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/bond.c b/ofproto/bond.c index >>> a4116588f..2dcfeda71 100644 >>> --- a/ofproto/bond.c >>> +++ b/ofproto/bond.c >>> @@ -672,8 +672,13 @@ out: >>> void >>> bond_member_set_may_enable(struct bond *bond, void *member_, bool >>> may_enable) { >>> + struct bond_member *member; >>> + >>> ovs_rwlock_wrlock(&rwlock); >>> - bond_member_lookup(bond, member_)->may_enable = may_enable; >>> + member = bond_member_lookup(bond, member_); >>> + if (member) { >>> + member->may_enable = may_enable; >>> + } >>> ovs_rwlock_unlock(&rwlock); >>> } >>> >>> >
diff --git a/ofproto/bond.c b/ofproto/bond.c index a4116588f..2dcfeda71 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -672,8 +672,13 @@ out: void bond_member_set_may_enable(struct bond *bond, void *member_, bool may_enable) { + struct bond_member *member; + ovs_rwlock_wrlock(&rwlock); - bond_member_lookup(bond, member_)->may_enable = may_enable; + member = bond_member_lookup(bond, member_); + if (member) { + member->may_enable = may_enable; + } ovs_rwlock_unlock(&rwlock); }
The function bond_member_lookup() could return NULL, the return value need to be checked. Addresses-Coverity: ("Dereference null return value") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- ofproto/bond.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)