| Message ID | 169755058144.686004.17081557391716788970.stgit@ebuild |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix some of Clang's static analyzer warnings. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Oct 17, 2023 at 03:49:41PM +0200, Eelco Chaudron wrote: > This patch fixes two 'Dead assignment' warnings, where the one in > count_common_prefix_run() is actually a bug where the set is in reverse order. Hi Eelco, I agree with the correctness of these changes, and that they address clang-tidy warnings. But if one is a bug should it be in a separate patch, with a fixes tag, and description of the problem that is being addressed? > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/ofp-monitor.c | 5 ++--- > lib/ofp-table.c | 2 +- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c > index c27733a52..aed5f0497 100644 > --- a/lib/ofp-monitor.c > +++ b/lib/ofp-monitor.c > @@ -962,13 +962,12 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update, > return 0; > } else if (update->event == OFPFME_PAUSED > || update->event == OFPFME_RESUMED) { > - struct ofp_flow_update_paused *ofup; > > - if (length != sizeof *ofup) { > + if (length != sizeof(struct ofp_flow_update_paused)) { > goto bad_len; > } > > - ofup = ofpbuf_pull(msg, sizeof *ofup); > + ofpbuf_pull(msg, sizeof(struct ofp_flow_update_paused)); > return 0; > } else if (update->event == OFPFME_INITIAL > || update->event == OFPFME_ADDED > diff --git a/lib/ofp-table.c b/lib/ofp-table.c > index a956754f2..f9bd3b7f9 100644 > --- a/lib/ofp-table.c > +++ b/lib/ofp-table.c > @@ -1416,7 +1416,7 @@ count_common_prefix_run(const char *ids[], size_t n, > if (!next) { > break; > } else if (next < extra_prefix_len) { > - next = extra_prefix_len; > + extra_prefix_len = next; > } > i++; > } > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 18 Oct 2023, at 12:50, Simon Horman wrote: > On Tue, Oct 17, 2023 at 03:49:41PM +0200, Eelco Chaudron wrote: >> This patch fixes two 'Dead assignment' warnings, where the one in >> count_common_prefix_run() is actually a bug where the set is in reverse order. > > Hi Eelco, > > I agree with the correctness of these changes, and that they > address clang-tidy warnings. But if one is a bug should it be in a > separate patch, with a fixes tag, and description of the > problem that is being addressed? ACK, makes sense, let me split up the patch. I found the problem when looking at the reported issue. >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/ofp-monitor.c | 5 ++--- >> lib/ofp-table.c | 2 +- >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c >> index c27733a52..aed5f0497 100644 >> --- a/lib/ofp-monitor.c >> +++ b/lib/ofp-monitor.c >> @@ -962,13 +962,12 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update, >> return 0; >> } else if (update->event == OFPFME_PAUSED >> || update->event == OFPFME_RESUMED) { >> - struct ofp_flow_update_paused *ofup; >> >> - if (length != sizeof *ofup) { >> + if (length != sizeof(struct ofp_flow_update_paused)) { >> goto bad_len; >> } >> >> - ofup = ofpbuf_pull(msg, sizeof *ofup); >> + ofpbuf_pull(msg, sizeof(struct ofp_flow_update_paused)); >> return 0; >> } else if (update->event == OFPFME_INITIAL >> || update->event == OFPFME_ADDED >> diff --git a/lib/ofp-table.c b/lib/ofp-table.c >> index a956754f2..f9bd3b7f9 100644 >> --- a/lib/ofp-table.c >> +++ b/lib/ofp-table.c >> @@ -1416,7 +1416,7 @@ count_common_prefix_run(const char *ids[], size_t n, >> if (!next) { >> break; >> } else if (next < extra_prefix_len) { >> - next = extra_prefix_len; >> + extra_prefix_len = next; >> } >> i++; >> } >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On 10/17/23 15:49, Eelco Chaudron wrote: > This patch fixes two 'Dead assignment' warnings, where the one in > count_common_prefix_run() is actually a bug where the set is in reverse order. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/ofp-monitor.c | 5 ++--- > lib/ofp-table.c | 2 +- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c > index c27733a52..aed5f0497 100644 > --- a/lib/ofp-monitor.c > +++ b/lib/ofp-monitor.c > @@ -962,13 +962,12 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update, > return 0; > } else if (update->event == OFPFME_PAUSED > || update->event == OFPFME_RESUMED) { > - struct ofp_flow_update_paused *ofup; > > - if (length != sizeof *ofup) { > + if (length != sizeof(struct ofp_flow_update_paused)) { > goto bad_len; > } > > - ofup = ofpbuf_pull(msg, sizeof *ofup); > + ofpbuf_pull(msg, sizeof(struct ofp_flow_update_paused)); Hmm. I find the previous code a little bit more readable. Maybe we can just silence the warning with OVS_UNUSED since this variable is indeed not used? What do you think? Best regards, Ilya Maximets.
On 18 Oct 2023, at 13:42, Ilya Maximets wrote: > On 10/17/23 15:49, Eelco Chaudron wrote: >> This patch fixes two 'Dead assignment' warnings, where the one in >> count_common_prefix_run() is actually a bug where the set is in reverse order. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/ofp-monitor.c | 5 ++--- >> lib/ofp-table.c | 2 +- >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c >> index c27733a52..aed5f0497 100644 >> --- a/lib/ofp-monitor.c >> +++ b/lib/ofp-monitor.c >> @@ -962,13 +962,12 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update, >> return 0; >> } else if (update->event == OFPFME_PAUSED >> || update->event == OFPFME_RESUMED) { >> - struct ofp_flow_update_paused *ofup; >> >> - if (length != sizeof *ofup) { >> + if (length != sizeof(struct ofp_flow_update_paused)) { >> goto bad_len; >> } >> >> - ofup = ofpbuf_pull(msg, sizeof *ofup); >> + ofpbuf_pull(msg, sizeof(struct ofp_flow_update_paused)); > > Hmm. I find the previous code a little bit more readable. > Maybe we can just silence the warning with OVS_UNUSED since > this variable is indeed not used? What do you think? I’m fine with either format, so if you prefer OVS_UNUSED, I’ll update the next rev. //Eelco
diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c index c27733a52..aed5f0497 100644 --- a/lib/ofp-monitor.c +++ b/lib/ofp-monitor.c @@ -962,13 +962,12 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update, return 0; } else if (update->event == OFPFME_PAUSED || update->event == OFPFME_RESUMED) { - struct ofp_flow_update_paused *ofup; - if (length != sizeof *ofup) { + if (length != sizeof(struct ofp_flow_update_paused)) { goto bad_len; } - ofup = ofpbuf_pull(msg, sizeof *ofup); + ofpbuf_pull(msg, sizeof(struct ofp_flow_update_paused)); return 0; } else if (update->event == OFPFME_INITIAL || update->event == OFPFME_ADDED diff --git a/lib/ofp-table.c b/lib/ofp-table.c index a956754f2..f9bd3b7f9 100644 --- a/lib/ofp-table.c +++ b/lib/ofp-table.c @@ -1416,7 +1416,7 @@ count_common_prefix_run(const char *ids[], size_t n, if (!next) { break; } else if (next < extra_prefix_len) { - next = extra_prefix_len; + extra_prefix_len = next; } i++; }
This patch fixes two 'Dead assignment' warnings, where the one in count_common_prefix_run() is actually a bug where the set is in reverse order. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/ofp-monitor.c | 5 ++--- lib/ofp-table.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)