[ovs-dev] [PATCH 3/3] smap: Return default on failure in smap_get_int/ullong.

Jan Scheurich jan.scheurich at ericsson.com
Wed Nov 29 14:59:22 UTC 2017


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 <jan.scheurich at ericsson.com>
Acked-by: Jan Scheurich <jan.scheurich at ericsson.com>

> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Ilya Maximets
> Sent: Wednesday, 29 November, 2017 11:51
> To: ovs-dev at openvswitch.org
> Cc: Ilya Maximets <i.maximets at samsung.com>; Heetae Ahn <heetae82.ahn at samsung.com>
> 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 <i.maximets at samsung.com>
> ---
>  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list