On 7/11/23 12:05, Eelco Chaudron wrote: > > > On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > >> This commit adds assertions in the functions `shash_count`, >> `simap_count`, and `smap_count` to ensure that the corresponding input >> struct pointer is not NULL. >> >> This ensures that if the return values of `shash_sort`, `simap_sort`, >> or `smap_sort` are NULL, then the following for loops would not attempt >> to access the pointer, which might result in segmentation faults or >> undefined behavior. >> >> Signed-off-by: James Raphael Tiovalen <[email protected]> >> Reviewed-by: Simon Horman <[email protected]> >> --- >> lib/shash.c | 2 ++ >> lib/simap.c | 2 ++ >> lib/smap.c | 1 + >> 3 files changed, 5 insertions(+) >> >> diff --git a/lib/shash.c b/lib/shash.c >> index a7b2c6458..2bfc8eb50 100644 >> --- a/lib/shash.c >> +++ b/lib/shash.c >> @@ -17,6 +17,7 @@ >> #include <config.h> >> #include "openvswitch/shash.h" >> #include "hash.h" >> +#include "util.h" >> >> static struct shash_node *shash_find__(const struct shash *, >> const char *name, size_t name_len, >> @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash) >> size_t >> shash_count(const struct shash *shash) >> { >> + ovs_assert(shash); > > My preference would be to return 0, in these instances. What do others think?
Calling these function with a NULL argument doesn't make much sense to me. free()-like functions should generally accept NULL pointers, but functions that actually do work on a datastructure shouldn't, IMO. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
