Hi Aaron,
On Mon, Apr 3, 2023 at 10:15 PM Aaron Conole <[email protected]> wrote:
>
> Sorry to comment on this so late - one thing I would also say can help
> with review is to separate the logical changes. For example, each flag
> from Coverity could be a separate patch in a cleanup series. It could
> help to keep things organized. Comments to follow.
>
Sure thing. I will split this into a patch series and I will group
similar changes into separate patches.
>
> There's a bit of intermingling of null checks here. Please stick to
> one style. Throughout the dp-packet.c file, there are positive tests
> (ie 'ovs_assert(data_dp)' would match the style more).
>
Got it. I will change null checks in this patch series to positive tests.
>
> Probably better to check this before we use dp_packet_data() in a
> calculation. It seems odd to check after we've set up dst.
>
Sure.
>
> Rather than another level of indentation here, can we not change the
> test condition to be:
>
> for(size_t j = 0; (nodes &&
> j < smap_count(&config)); j++) {
> }
>
> Similar comment in other places.
>
> An alternative is to change smap_count() to return '0' if smap is NULL,
> or assert there (and we can do this for shash as well).
>
I will use the approach of changing `smap_count()` to return `0` if
`smap_is_empty`. I will do that for `shash` and `simap` as well and
update the relevant changes.
>
> This can be expressed closer to the first assert in the function:
>
> assert(dp_packet_data(buf));
>
> Or possibly(? untested) change the assert from dp_packet_is_eth to
> assert(dp_packet_eth(buf));
>
I have tested it and changing the assert from `dp_packet_is_eth` to
`assert(dp_packet_eth(buf))` would not work and would break some
tests. I will move the `assert(dp_packet_data(buf))` nearer to the
first assert instead.
>
> Can we instead change the ofpbuf_at to ofpbuf_at_assert() ?
>
Sure.
>
> Same here
>
Sure.
>
> Maybe instead of this, we should change the SFL_ALLOC define to be
> xzalloc which will guarantee a zero'd buffer that is != NULL and simply
> leave the assert.
>
Sure, will implement this in the next version of the patch.
>
> Indentation on this is wrong.
>
Got it, will fix it in the next version.
>
> Same here.
>
Got it.
>
> Consider we should move this type of check to shash steal, or possibly
> change shash_steal to assert node != NULL (although, shash_steal doesn't
> seem to have any other users in-tree but it is a published API).
>
Sure. I will do a check instead of an assert since `shash_steal` is a
published API.
>
> I wonder how coverity's model broke here. It seems to inconsistently
> flag the optarg tests? I didn't check every last one but it does seem
> that some which are marked "required_arg" should should be generating
> the correct flags for getopt are skipped and some aren't. Maybe this
> needs some additional investigation?
>
Hmm, I will not include these assertions for `optarg` for this patch
series. Further investigation could be done separately, and if these
are false positives, we can indicate them as so to Coverity.
>
> Instead, consider using an ovs_assert() where the code calls
> shash_find(), which should make reading this a bit more pleasant.
>
Sure.
Best regards,
James Raphael Tiovalen
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
