Hello Aaron,
On 25 January 2016 at 10:28, Aaron Conole <acon...@redhat.com> wrote:
> Mauricio Vasquez B <mauricio.vasquezber...@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.vasquezber...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
