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...
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
