| Message ID | 1452450492-7923-1-git-send-email-mauricio.vasquezbernal@studenti.polito.it |
|---|---|
| State | Not Applicable |
| Headers | show |
> -----Original Message----- > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Mauricio Vasquez > B > Sent: Sunday, January 10, 2016 6:28 PM > To: dev@openvswitch.org > Cc: aconole@bytheb.org > Subject: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name length for > dpdkr ports > > A ring name length of 10 characters is not enough for dpdkr ports > starting from dpdkr10, then it is increased to RTE_RING_NAMESIZE > characters. Looks good to me. There's some existing headroom for name length in ring_client.c but you may want to also increase it as part of this change now that even larger numbers are possible? It should work up to 9999 as is, so it would be just to catch someone using magic numbers. > > Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> > --- > v2: > - Use RTE_RING_NAMESIZE instead of a numerical constant. > > lib/netdev-dpdk.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index b209df2..90512aa 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1921,7 +1921,7 @@ dpdk_ring_create(const char dev_name[], unsigned int > port_no, > unsigned int *eth_port_id) > { > struct dpdk_ring *ivshmem; > - char ring_name[10]; > + char ring_name[RTE_RING_NAMESIZE]; > int err; > > ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); > @@ -1930,7 +1930,7 @@ dpdk_ring_create(const char dev_name[], unsigned int > port_no, > } > > /* XXX: Add support for multiquque ring. */ > - err = snprintf(ring_name, 10, "%s_tx", dev_name); > + err = snprintf(ring_name, sizeof(ring_name), "%s_tx", dev_name); > if (err < 0) { > return -err; > } > @@ -1943,7 +1943,7 @@ dpdk_ring_create(const char dev_name[], unsigned int > port_no, > return ENOMEM; > } > > - err = snprintf(ring_name, 10, "%s_rx", dev_name); > + err = snprintf(ring_name, sizeof(ring_name), "%s_rx", dev_name); > if (err < 0) { > return -err; > } > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hello Kevin, It can only work up to 255 actually, notice that the port number is first parsed and saved in the variable "uint8_t client_id", then the type of this variable should be changed too. Additionally, the port number in ovs is saved in the member "int user_port_id;" of the dpdk_ring structure. My proposal would be to change the type of both variables to 'unsigned int' and add some error handling in the case overflow happens during the parsing. What do you think? On 11/01/2016, Traynor, Kevin <kevin.traynor@intel.com> wrote: > >> -----Original Message----- >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Mauricio >> Vasquez >> B >> Sent: Sunday, January 10, 2016 6:28 PM >> To: dev@openvswitch.org >> Cc: aconole@bytheb.org >> Subject: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name length >> for >> dpdkr ports >> >> A ring name length of 10 characters is not enough for dpdkr ports >> starting from dpdkr10, then it is increased to RTE_RING_NAMESIZE >> characters. > > Looks good to me. There's some existing headroom for name length in > ring_client.c > but you may want to also increase it as part of this change now that even > larger > numbers are possible? It should work up to 9999 as is, so it would be just > to catch > someone using magic numbers. > >> >> Signed-off-by: Mauricio Vasquez B >> <mauricio.vasquezbernal@studenti.polito.it> >> --- >> v2: >> - Use RTE_RING_NAMESIZE instead of a numerical constant. >> >> lib/netdev-dpdk.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index b209df2..90512aa 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1921,7 +1921,7 @@ dpdk_ring_create(const char dev_name[], unsigned >> int >> port_no, >> unsigned int *eth_port_id) >> { >> struct dpdk_ring *ivshmem; >> - char ring_name[10]; >> + char ring_name[RTE_RING_NAMESIZE]; >> int err; >> >> ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); >> @@ -1930,7 +1930,7 @@ dpdk_ring_create(const char dev_name[], unsigned >> int >> port_no, >> } >> >> /* XXX: Add support for multiquque ring. */ >> - err = snprintf(ring_name, 10, "%s_tx", dev_name); >> + err = snprintf(ring_name, sizeof(ring_name), "%s_tx", dev_name); >> if (err < 0) { >> return -err; >> } >> @@ -1943,7 +1943,7 @@ dpdk_ring_create(const char dev_name[], unsigned >> int >> port_no, >> return ENOMEM; >> } >> >> - err = snprintf(ring_name, 10, "%s_rx", dev_name); >> + err = snprintf(ring_name, sizeof(ring_name), "%s_rx", dev_name); >> if (err < 0) { >> return -err; >> } >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >
> -----Original Message----- > From: Mauricio Vásquez [mailto:mauricio.vasquezbernal@studenti.polito.it] > Sent: Thursday, January 14, 2016 9:15 PM > To: Traynor, Kevin > Cc: dev@openvswitch.org; aconole@bytheb.org > Subject: Re: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name length > for dpdkr ports > > Hello Kevin, > > It can only work up to 255 actually, notice that the port number is > first parsed and saved in the variable "uint8_t client_id", then the > type of this variable should be changed too. Additionally, the port > number in ovs is saved in the member "int user_port_id;" of the > dpdk_ring structure. Hi - yes, you're right! I had just checked the length of the buffer, not the client_id. > > My proposal would be to change the type of both variables to 'unsigned > int' and add some error handling in the case overflow happens during > the parsing. > > What do you think? That sounds good. I'm not really sure how likely it is that someone would decide to use a very large number and ring_client.c is only a test program but if it's simple to put in the check it's no harm to have it as good reference code. > > > On 11/01/2016, Traynor, Kevin <kevin.traynor@intel.com> wrote: > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Mauricio > >> Vasquez > >> B > >> Sent: Sunday, January 10, 2016 6:28 PM > >> To: dev@openvswitch.org > >> Cc: aconole@bytheb.org > >> Subject: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name length > >> for > >> dpdkr ports > >> > >> A ring name length of 10 characters is not enough for dpdkr ports > >> starting from dpdkr10, then it is increased to RTE_RING_NAMESIZE > >> characters. > > > > Looks good to me. There's some existing headroom for name length in > > ring_client.c > > but you may want to also increase it as part of this change now that even > > larger > > numbers are possible? It should work up to 9999 as is, so it would be just > > to catch > > someone using magic numbers. > > > >> > >> Signed-off-by: Mauricio Vasquez B > >> <mauricio.vasquezbernal@studenti.polito.it> > >> --- > >> v2: > >> - Use RTE_RING_NAMESIZE instead of a numerical constant. > >> > >> lib/netdev-dpdk.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index b209df2..90512aa 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -1921,7 +1921,7 @@ dpdk_ring_create(const char dev_name[], unsigned > >> int > >> port_no, > >> unsigned int *eth_port_id) > >> { > >> struct dpdk_ring *ivshmem; > >> - char ring_name[10]; > >> + char ring_name[RTE_RING_NAMESIZE]; > >> int err; > >> > >> ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); > >> @@ -1930,7 +1930,7 @@ dpdk_ring_create(const char dev_name[], unsigned > >> int > >> port_no, > >> } > >> > >> /* XXX: Add support for multiquque ring. */ > >> - err = snprintf(ring_name, 10, "%s_tx", dev_name); > >> + err = snprintf(ring_name, sizeof(ring_name), "%s_tx", dev_name); > >> if (err < 0) { > >> return -err; > >> } > >> @@ -1943,7 +1943,7 @@ dpdk_ring_create(const char dev_name[], unsigned > >> int > >> port_no, > >> return ENOMEM; > >> } > >> > >> - err = snprintf(ring_name, 10, "%s_rx", dev_name); > >> + err = snprintf(ring_name, sizeof(ring_name), "%s_rx", dev_name); > >> if (err < 0) { > >> return -err; > >> } > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev > >
I sent this patch as part of a patch series: http://openvswitch.org/pipermail/dev/2016-January/064968.html. Thank you very much. On 15 January 2016 at 10:17, Traynor, Kevin <kevin.traynor@intel.com> wrote: > > > -----Original Message----- > > From: Mauricio Vásquez [mailto:mauricio.vasquezbernal@studenti.polito.it > ] > > Sent: Thursday, January 14, 2016 9:15 PM > > To: Traynor, Kevin > > Cc: dev@openvswitch.org; aconole@bytheb.org > > Subject: Re: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name > length > > for dpdkr ports > > > > Hello Kevin, > > > > It can only work up to 255 actually, notice that the port number is > > first parsed and saved in the variable "uint8_t client_id", then the > > type of this variable should be changed too. Additionally, the port > > number in ovs is saved in the member "int user_port_id;" of the > > dpdk_ring structure. > > Hi - yes, you're right! I had just checked the length of the buffer, not > the > client_id. > > > > > My proposal would be to change the type of both variables to 'unsigned > > int' and add some error handling in the case overflow happens during > > the parsing. > > > > What do you think? > > That sounds good. I'm not really sure how likely it is that someone would > decide to use a very large number and ring_client.c is only a test program > but if it's simple to put in the check it's no harm to have it as good > reference > code. > > > > > > > On 11/01/2016, Traynor, Kevin <kevin.traynor@intel.com> wrote: > > > > > >> -----Original Message----- > > >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Mauricio > > >> Vasquez > > >> B > > >> Sent: Sunday, January 10, 2016 6:28 PM > > >> To: dev@openvswitch.org > > >> Cc: aconole@bytheb.org > > >> Subject: [ovs-dev] [PATCH v2] lib/netdev-dpdk: increase ring name > length > > >> for > > >> dpdkr ports > > >> > > >> A ring name length of 10 characters is not enough for dpdkr ports > > >> starting from dpdkr10, then it is increased to RTE_RING_NAMESIZE > > >> characters. > > > > > > Looks good to me. There's some existing headroom for name length in > > > ring_client.c > > > but you may want to also increase it as part of this change now that > even > > > larger > > > numbers are possible? It should work up to 9999 as is, so it would be > just > > > to catch > > > someone using magic numbers. > > > > > >> > > >> Signed-off-by: Mauricio Vasquez B > > >> <mauricio.vasquezbernal@studenti.polito.it> > > >> --- > > >> v2: > > >> - Use RTE_RING_NAMESIZE instead of a numerical constant. > > >> > > >> lib/netdev-dpdk.c | 6 +++--- > > >> 1 file changed, 3 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > >> index b209df2..90512aa 100644 > > >> --- a/lib/netdev-dpdk.c > > >> +++ b/lib/netdev-dpdk.c > > >> @@ -1921,7 +1921,7 @@ dpdk_ring_create(const char dev_name[], unsigned > > >> int > > >> port_no, > > >> unsigned int *eth_port_id) > > >> { > > >> struct dpdk_ring *ivshmem; > > >> - char ring_name[10]; > > >> + char ring_name[RTE_RING_NAMESIZE]; > > >> int err; > > >> > > >> ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); > > >> @@ -1930,7 +1930,7 @@ dpdk_ring_create(const char dev_name[], unsigned > > >> int > > >> port_no, > > >> } > > >> > > >> /* XXX: Add support for multiquque ring. */ > > >> - err = snprintf(ring_name, 10, "%s_tx", dev_name); > > >> + err = snprintf(ring_name, sizeof(ring_name), "%s_tx", dev_name); > > >> if (err < 0) { > > >> return -err; > > >> } > > >> @@ -1943,7 +1943,7 @@ dpdk_ring_create(const char dev_name[], unsigned > > >> int > > >> port_no, > > >> return ENOMEM; > > >> } > > >> > > >> - err = snprintf(ring_name, 10, "%s_rx", dev_name); > > >> + err = snprintf(ring_name, sizeof(ring_name), "%s_rx", dev_name); > > >> if (err < 0) { > > >> return -err; > > >> } > > >> -- > > >> 1.9.1 > > >> > > >> _______________________________________________ > > >> dev mailing list > > >> dev@openvswitch.org > > >> http://openvswitch.org/mailman/listinfo/dev > > > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index b209df2..90512aa 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1921,7 +1921,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, unsigned int *eth_port_id) { struct dpdk_ring *ivshmem; - char ring_name[10]; + char ring_name[RTE_RING_NAMESIZE]; int err; ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); @@ -1930,7 +1930,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, } /* XXX: Add support for multiquque ring. */ - err = snprintf(ring_name, 10, "%s_tx", dev_name); + err = snprintf(ring_name, sizeof(ring_name), "%s_tx", dev_name); if (err < 0) { return -err; } @@ -1943,7 +1943,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, return ENOMEM; } - err = snprintf(ring_name, 10, "%s_rx", dev_name); + err = snprintf(ring_name, sizeof(ring_name), "%s_rx", dev_name); if (err < 0) { return -err; }
A ring name length of 10 characters is not enough for dpdkr ports starting from dpdkr10, then it is increased to RTE_RING_NAMESIZE characters. Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> --- v2: - Use RTE_RING_NAMESIZE instead of a numerical constant. lib/netdev-dpdk.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)