| Message ID | 20240523191152.589605-6-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/6] netdev-offload: Fix null pointer dereference' warnings on dump creation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/23/24 21:11, Mike Pattrick wrote: > Clang's static analyzer noted that the output from > netdev_linux_get_speed_locked can be checked even if this function > doesn't set any values. > > Now we always set those values to a sane default in all cases. > > Fixes: 6240c0b4c80e ("netdev: Add netdev_get_speed() to netdev API.") This doesn't look like the right tag. The issue, AFAICT, was introduced when QoS functions started to use netdev_linux_get_speed_locked() without checking the return value. That happened much later. Or am I missing something? The alternative would be to fix those functions instead, but this approach is also fine. > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-linux.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 8b855bdc4..83c19618c 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2726,6 +2726,8 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev, > uint32_t *current, uint32_t *max) > { > if (netdev_linux_netnsid_is_remote(netdev)) { > + *current = 0; > + *max = 0; > return EOPNOTSUPP; > } > > @@ -2735,6 +2737,9 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev, > ? 0 : netdev->current_speed; > *max = MIN(UINT32_MAX, > netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); > + } else { > + *current = 0; > + *max = 0; Please, squash these into one line as netdev_get_speed() does. Same for the other instance. > } > return netdev->get_features_error; > } Best regards, Ilya Maximets.
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 8b855bdc4..83c19618c 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2726,6 +2726,8 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev, uint32_t *current, uint32_t *max) { if (netdev_linux_netnsid_is_remote(netdev)) { + *current = 0; + *max = 0; return EOPNOTSUPP; } @@ -2735,6 +2737,9 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev, ? 0 : netdev->current_speed; *max = MIN(UINT32_MAX, netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); + } else { + *current = 0; + *max = 0; } return netdev->get_features_error; }
Clang's static analyzer noted that the output from netdev_linux_get_speed_locked can be checked even if this function doesn't set any values. Now we always set those values to a sane default in all cases. Fixes: 6240c0b4c80e ("netdev: Add netdev_get_speed() to netdev API.") Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-linux.c | 5 +++++ 1 file changed, 5 insertions(+)