On 29.11.2017 17:59, Jan Scheurich wrote: > Thanks for fixing this. Applying the default value is better than silently > applying 0. > Even better would be to flag an error in this case...
Thanks for review and testing. I hope that someday we will have type checking support from the ovsdb side for such maps like 'other_config'. This could prevent storing completely wrong values in database. Ben, what do you think? Is it possible? Best regards, Ilya Maximets. > > Tested-by: Jan Scheurich <[email protected]> > Acked-by: Jan Scheurich <[email protected]> > >> -----Original Message----- >> From: [email protected] >> [mailto:[email protected]] On Behalf Of Ilya Maximets >> Sent: Wednesday, 29 November, 2017 11:51 >> To: [email protected] >> Cc: Ilya Maximets <[email protected]>; Heetae Ahn >> <[email protected]> >> Subject: [ovs-dev] [PATCH 3/3] smap: Return default on failure in >> smap_get_int/ullong. >> >> Currently smap_get_int/ullong doesn't check any conversion errors. >> Most implementations of atoi/strtoull return 0 in case of failure. >> >> This leads to returning zero in case of wrongly set database values. >> For example, commands >> >> ovs-vsctl set interface iface options:key=\"\" >> ovs-vsctl set interface iface options:key=qwe123 >> ovs-vsctl set interface iface options:key=abc >> >> will have exactly same effect as >> >> ovs-vsctl set interface iface options:key=0 >> >> in case where 'key' is an integer option of the iface. >> Can be checked with 'other_config:emc-insert-inv-prob' or other >> integer 'options' and 'other_config's. >> >> 0 could be not a default and not safe value for many options and >> it'll be better to return default value instead if any. >> >> Conversion functions from 'util' library used to provide proper >> error handling. >> >> Signed-off-by: Ilya Maximets <[email protected]> >> --- >> lib/smap.c | 25 +++++++++++++++++++------ >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/lib/smap.c b/lib/smap.c >> index 0a5e54a..6c6717c 100644 >> --- a/lib/smap.c >> +++ b/lib/smap.c >> @@ -20,6 +20,7 @@ >> #include "hash.h" >> #include "openvswitch/json.h" >> #include "packets.h" >> +#include "util.h" >> #include "uuid.h" >> >> static struct smap_node *smap_add__(struct smap *, char *, void *, >> @@ -221,25 +222,37 @@ smap_get_bool(const struct smap *smap, const char >> *key, bool def) >> } >> } >> >> -/* Gets the value associated with 'key' in 'smap' and converts it to an int >> - * using atoi(). If 'key' is not in 'smap', returns 'def'. */ >> +/* Gets the value associated with 'key' in 'smap' and converts it to an int. >> + * If 'key' is not in 'smap' or a valid integer can't be parsed from it's >> + * value, returns 'def'. */ >> int >> smap_get_int(const struct smap *smap, const char *key, int def) >> { >> const char *value = smap_get(smap, key); >> + int i_value; >> >> - return value ? atoi(value) : def; >> + if (!value || !str_to_int(value, 10, &i_value)) { >> + return def; >> + } >> + >> + return i_value; >> } >> >> -/* Gets the value associated with 'key' in 'smap' and converts it to an int >> - * using strtoull(). If 'key' is not in 'smap', returns 'def'. */ >> +/* Gets the value associated with 'key' in 'smap' and converts it to an >> + * unsigned long long. If 'key' is not in 'smap' or a valid number can't be >> + * parsed from it's value, returns 'def'. */ >> unsigned long long int >> smap_get_ullong(const struct smap *smap, const char *key, >> unsigned long long def) >> { >> const char *value = smap_get(smap, key); >> + unsigned long long ull_value; >> + >> + if (!value || !str_to_ullong(value, 10, &ull_value)) { >> + return def; >> + } >> >> - return value ? strtoull(value, NULL, 10) : def; >> + return ull_value; >> } >> >> /* Gets the value associated with 'key' in 'smap' and converts it to a UUID >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
