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

[Mauricio Vásquez]

Hello Aaron,

On 25 January 2016 at 10:28, Aaron Conole <aconole at redhat.com> wrote:

> 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).
>
>
I used this temporal variable because the port_no is a 'unsigned int'
variable
and there is not any strto* function to convert to that type, assigning the
return
value directly to port_no could cause an overflow that can not be detected.

I realized that with this patch port numbers as "dpdkr 5" (notice the blank
space), are considered valid.
I'll send a new patch.


> > +    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.
>
>
It depends on the user, I think supporting only decimal notation is enough,
it is the natural way of numbering things.


> > +    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;
> >  }
>

Thank you very much for the review.