[ovs-dev] [PATCH 1/3] lib/netdev-dpdk: make device name parsing more robust

[Aaron Conole]

Mauricio Vasquez B <mauricio.vasquezbernal at studenti.polito.it> writes:

> Current implementation of dpdk_dev_parse_name does not perform a robust
> error handling, port names as "dpdkr" or "dpdkr1x" are considered valid.

Mauricio, thanks for the patch!

> Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal at studenti.polito.it>
> ---
>  lib/netdev-dpdk.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index de7e488..ac81f2f 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -19,6 +19,7 @@
>  #include <string.h>
>  #include <signal.h>
>  #include <stdlib.h>
> +#include <limits.h>
>  #include <pthread.h>
>  #include <config.h>
>  #include <errno.h>
> @@ -187,7 +188,7 @@ struct dpdk_ring {
>      /* For the client rings */
>      struct rte_ring *cring_tx;
>      struct rte_ring *cring_rx;
> -    int user_port_id; /* User given port no, parsed from port name */
> +    unsigned int user_port_id; /* User given port no, parsed from port name */
>      int eth_port_id; /* ethernet device port id */
>      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>  };
> @@ -641,13 +642,30 @@ dpdk_dev_parse_name(const char dev_name[], const char prefix[],
>                      unsigned int *port_no)
>  {
>      const char *cport;
> +    unsigned long port;

port here is unsigned, but you use it with strtol (I know the original
code did this as well - just curious). Also, why introduce it at all? Is
it just to avoid assigning port_no on error? If so, I suggest using long
type and check it's value for less-than 0 as well (so that dpdkr-123
would also be an error).

> +    char *endptr;
>  
>      if (strncmp(dev_name, prefix, strlen(prefix))) {
>          return ENODEV;
>      }
>  
> +    errno = 0;
>      cport = dev_name + strlen(prefix);
> -    *port_no = strtol(cport, NULL, 0); /* string must be null terminated */
> +    port = strtol(cport, &endptr, 10);

Would there be any value in a non-decimal input on the port number ever?
Just curious, since I also don't see one.

> +    if(errno != 0) {
> +        return errno;
> +    }
> +
> +    if(endptr == NULL || *endptr != '\0' || endptr == cport) {
> +        return ENODEV;
> +    }
> +
> +    if(port > UINT_MAX) {
> +        return ENODEV;
> +    }
> +
> +    *port_no = port;
>      return 0;
>  }