[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
On Mon, Jan 18, 2016 at 03:38:43PM +0100, Olivier MATZ wrote: > Hi, > > On 01/15/2016 10:00 AM, Panu Matilainen wrote: > diff --git a/lib/librte_cmdline/cmdline_rdline.h > b/lib/librte_cmdline/cmdline_rdline.h > index b9aad9b..72e2dad 100644 > --- a/lib/librte_cmdline/cmdline_rdline.h > +++ b/lib/librte_cmdline/cmdline_rdline.h > @@ -93,7 +93,7 @@ extern "C" { > #endif > > /* configuration */ > -#define RDLINE_BUF_SIZE 256 > +#define RDLINE_BUF_SIZE 512 > #define RDLINE_PROMPT_SIZE 32 > #define RDLINE_VT100_BUF_SIZE 8 > #define RDLINE_HISTORY_BUF_SIZE BUFSIZ > >>> > >>> Having to break a library ABI for a change like this is a bit > >>> ridiculous. > >> > >> Sure, but John McNamara needed it to handle flow director with IPv6[1]. > >> > >> For my part, I was needing it to manipulate the RETA table, but as I > >> wrote in the cover letter, it ends by breaking other commands. > >> Olivier Matz, has proposed another way to handle long commands lines[2], > >> it could be a good idea to go on this direction. > >> > >> For RETA situation, we already discussed on a new API, but for now, I > >> do not have time for it (and as it is another ABI breakage it could only > >> be done for 16.07 or 2.4)[3]. > >> > >> If this patch is no more needed we can just drop it, for that I would > >> like to have the point of view from John. > > > > Note that I was not objecting to the patch as such, I can easily see 256 > > characters not being enough for commandline buffer. > > > > I was merely noting that having to break an ABI to increase an > > effectively internal buffer size is a sign of a, um, less-than-optimal > > library design. > > You are right about the cmdline ABI. Changing this buffer size > should not imply an ABI change. I'll try to find some time to > investigate this issue. > > Another question we could raise is: should we export the API of > librte_cmdline to external applications? Now that baremetal dpdk is > not supported, having this library in dpdk is probably useless as > we can surely find standard replacements for it. A first step could > be to mark it as "internal". > > About the patch N?lio's patch itself, I'm not so convinced having more > than 256 characters is absolutely required, and I would prefer to see > the commands beeing reworked to be more human-readable. On the other > hand, the ABI breakage was announced so there is no reason to nack > this patch now. > > Regards, > Olivier John, What is your position about this patch? Is it still needed? Regards, -- N?lio Laranjeiro 6WIND
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
> -Original Message- > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > Sent: Friday, February 26, 2016 3:17 PM > To: Mcnamara, John > Cc: Olivier MATZ ; Panu Matilainen > ; dev at dpdk.org; Zhang, Helin intel.com>; > thomas.monjalon at 6wind.com; Lu, Wenzhuo ; > olgas at mellanox.com > Subject: Re: [dpdk-dev] [PATCH v2 1/3] cmdline: increase command line > buffer > > > What is your position about this patch? > Is it still needed? Yes. It is still required. Acked-by: John McNamara
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
Hi, On 01/15/2016 10:00 AM, Panu Matilainen wrote: diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h index b9aad9b..72e2dad 100644 --- a/lib/librte_cmdline/cmdline_rdline.h +++ b/lib/librte_cmdline/cmdline_rdline.h @@ -93,7 +93,7 @@ extern "C" { #endif /* configuration */ -#define RDLINE_BUF_SIZE 256 +#define RDLINE_BUF_SIZE 512 #define RDLINE_PROMPT_SIZE 32 #define RDLINE_VT100_BUF_SIZE 8 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ >>> >>> Having to break a library ABI for a change like this is a bit >>> ridiculous. >> >> Sure, but John McNamara needed it to handle flow director with IPv6[1]. >> >> For my part, I was needing it to manipulate the RETA table, but as I >> wrote in the cover letter, it ends by breaking other commands. >> Olivier Matz, has proposed another way to handle long commands lines[2], >> it could be a good idea to go on this direction. >> >> For RETA situation, we already discussed on a new API, but for now, I >> do not have time for it (and as it is another ABI breakage it could only >> be done for 16.07 or 2.4)[3]. >> >> If this patch is no more needed we can just drop it, for that I would >> like to have the point of view from John. > > Note that I was not objecting to the patch as such, I can easily see 256 > characters not being enough for commandline buffer. > > I was merely noting that having to break an ABI to increase an > effectively internal buffer size is a sign of a, um, less-than-optimal > library design. You are right about the cmdline ABI. Changing this buffer size should not imply an ABI change. I'll try to find some time to investigate this issue. Another question we could raise is: should we export the API of librte_cmdline to external applications? Now that baremetal dpdk is not supported, having this library in dpdk is probably useless as we can surely find standard replacements for it. A first step could be to mark it as "internal". About the patch N?lio's patch itself, I'm not so convinced having more than 256 characters is absolutely required, and I would prefer to see the commands beeing reworked to be more human-readable. On the other hand, the ABI breakage was announced so there is no reason to nack this patch now. Regards, Olivier
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
On 01/15/2016 10:44 AM, N?lio Laranjeiro wrote: > On Tue, Jan 12, 2016 at 02:46:07PM +0200, Panu Matilainen wrote: >> On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote: >>> Allow long command lines in testpmd (like flow director with IPv6, ...). >>> >>> Signed-off-by: John McNamara >>> Signed-off-by: Nelio Laranjeiro >>> --- >>> doc/guides/rel_notes/deprecation.rst | 5 - >>> lib/librte_cmdline/cmdline_rdline.h | 2 +- >>> 2 files changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/doc/guides/rel_notes/deprecation.rst >>> b/doc/guides/rel_notes/deprecation.rst >>> index e94d4a2..9cb288c 100644 >>> --- a/doc/guides/rel_notes/deprecation.rst >>> +++ b/doc/guides/rel_notes/deprecation.rst >>> @@ -44,8 +44,3 @@ Deprecation Notices >>> and table action handlers will be updated: >>> the pipeline parameter will be added, the packets mask parameter will be >>> either removed (for input port action handler) or made input-only. >>> - >>> -* ABI changes are planned in cmdline buffer size to allow the use of long >>> - commands (such as RETA update in testpmd). This should impact >>> - CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE. >>> - It should be integrated in release 2.3. >>> diff --git a/lib/librte_cmdline/cmdline_rdline.h >>> b/lib/librte_cmdline/cmdline_rdline.h >>> index b9aad9b..72e2dad 100644 >>> --- a/lib/librte_cmdline/cmdline_rdline.h >>> +++ b/lib/librte_cmdline/cmdline_rdline.h >>> @@ -93,7 +93,7 @@ extern "C" { >>> #endif >>> >>> /* configuration */ >>> -#define RDLINE_BUF_SIZE 256 >>> +#define RDLINE_BUF_SIZE 512 >>> #define RDLINE_PROMPT_SIZE 32 >>> #define RDLINE_VT100_BUF_SIZE 8 >>> #define RDLINE_HISTORY_BUF_SIZE BUFSIZ >> >> Having to break a library ABI for a change like this is a bit ridiculous. > > Sure, but John McNamara needed it to handle flow director with IPv6[1]. > > For my part, I was needing it to manipulate the RETA table, but as I > wrote in the cover letter, it ends by breaking other commands. > Olivier Matz, has proposed another way to handle long commands lines[2], > it could be a good idea to go on this direction. > > For RETA situation, we already discussed on a new API, but for now, I > do not have time for it (and as it is another ABI breakage it could only > be done for 16.07 or 2.4)[3]. > > If this patch is no more needed we can just drop it, for that I would > like to have the point of view from John. Note that I was not objecting to the patch as such, I can easily see 256 characters not being enough for commandline buffer. I was merely noting that having to break an ABI to increase an effectively internal buffer size is a sign of a, um, less-than-optimal library design. Apologies if I wasn't clear about that, - Panu -
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
On Tue, Jan 12, 2016 at 02:46:07PM +0200, Panu Matilainen wrote: > On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote: > >Allow long command lines in testpmd (like flow director with IPv6, ...). > > > >Signed-off-by: John McNamara > >Signed-off-by: Nelio Laranjeiro > >--- > > doc/guides/rel_notes/deprecation.rst | 5 - > > lib/librte_cmdline/cmdline_rdline.h | 2 +- > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > >diff --git a/doc/guides/rel_notes/deprecation.rst > >b/doc/guides/rel_notes/deprecation.rst > >index e94d4a2..9cb288c 100644 > >--- a/doc/guides/rel_notes/deprecation.rst > >+++ b/doc/guides/rel_notes/deprecation.rst > >@@ -44,8 +44,3 @@ Deprecation Notices > >and table action handlers will be updated: > >the pipeline parameter will be added, the packets mask parameter will be > >either removed (for input port action handler) or made input-only. > >- > >-* ABI changes are planned in cmdline buffer size to allow the use of long > >- commands (such as RETA update in testpmd). This should impact > >- CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE. > >- It should be integrated in release 2.3. > >diff --git a/lib/librte_cmdline/cmdline_rdline.h > >b/lib/librte_cmdline/cmdline_rdline.h > >index b9aad9b..72e2dad 100644 > >--- a/lib/librte_cmdline/cmdline_rdline.h > >+++ b/lib/librte_cmdline/cmdline_rdline.h > >@@ -93,7 +93,7 @@ extern "C" { > > #endif > > > > /* configuration */ > >-#define RDLINE_BUF_SIZE 256 > >+#define RDLINE_BUF_SIZE 512 > > #define RDLINE_PROMPT_SIZE 32 > > #define RDLINE_VT100_BUF_SIZE 8 > > #define RDLINE_HISTORY_BUF_SIZE BUFSIZ > > Having to break a library ABI for a change like this is a bit ridiculous. Sure, but John McNamara needed it to handle flow director with IPv6[1]. For my part, I was needing it to manipulate the RETA table, but as I wrote in the cover letter, it ends by breaking other commands. Olivier Matz, has proposed another way to handle long commands lines[2], it could be a good idea to go on this direction. For RETA situation, we already discussed on a new API, but for now, I do not have time for it (and as it is another ABI breakage it could only be done for 16.07 or 2.4)[3]. If this patch is no more needed we can just drop it, for that I would like to have the point of view from John. > > I didn't try it so could be wrong, but based on a quick look, struct rdline > could easily be made opaque to consumers by just adding functions for > allocating and freeing it. > > - Panu - > [1] http://dpdk.org/ml/archives/dev/2015-November/027643.html [2] http://dpdk.org/ml/archives/dev/2015-November/028557.html [3] http://dpdk.org/ml/archives/dev/2015-October/025294.html -- N?lio Laranjeiro 6WIND
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
On 01/12/2016 12:49 PM, Nelio Laranjeiro wrote: > Allow long command lines in testpmd (like flow director with IPv6, ...). > > Signed-off-by: John McNamara > Signed-off-by: Nelio Laranjeiro > --- > doc/guides/rel_notes/deprecation.rst | 5 - > lib/librte_cmdline/cmdline_rdline.h | 2 +- > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/doc/guides/rel_notes/deprecation.rst > b/doc/guides/rel_notes/deprecation.rst > index e94d4a2..9cb288c 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -44,8 +44,3 @@ Deprecation Notices > and table action handlers will be updated: > the pipeline parameter will be added, the packets mask parameter will be > either removed (for input port action handler) or made input-only. > - > -* ABI changes are planned in cmdline buffer size to allow the use of long > - commands (such as RETA update in testpmd). This should impact > - CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE. > - It should be integrated in release 2.3. > diff --git a/lib/librte_cmdline/cmdline_rdline.h > b/lib/librte_cmdline/cmdline_rdline.h > index b9aad9b..72e2dad 100644 > --- a/lib/librte_cmdline/cmdline_rdline.h > +++ b/lib/librte_cmdline/cmdline_rdline.h > @@ -93,7 +93,7 @@ extern "C" { > #endif > > /* configuration */ > -#define RDLINE_BUF_SIZE 256 > +#define RDLINE_BUF_SIZE 512 > #define RDLINE_PROMPT_SIZE 32 > #define RDLINE_VT100_BUF_SIZE 8 > #define RDLINE_HISTORY_BUF_SIZE BUFSIZ Having to break a library ABI for a change like this is a bit ridiculous. I didn't try it so could be wrong, but based on a quick look, struct rdline could easily be made opaque to consumers by just adding functions for allocating and freeing it. - Panu -
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
Allow long command lines in testpmd (like flow director with IPv6, ...). Signed-off-by: John McNamara Signed-off-by: Nelio Laranjeiro --- doc/guides/rel_notes/deprecation.rst | 5 - lib/librte_cmdline/cmdline_rdline.h | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst index e94d4a2..9cb288c 100644 --- a/doc/guides/rel_notes/deprecation.rst +++ b/doc/guides/rel_notes/deprecation.rst @@ -44,8 +44,3 @@ Deprecation Notices and table action handlers will be updated: the pipeline parameter will be added, the packets mask parameter will be either removed (for input port action handler) or made input-only. - -* ABI changes are planned in cmdline buffer size to allow the use of long - commands (such as RETA update in testpmd). This should impact - CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE. - It should be integrated in release 2.3. diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h index b9aad9b..72e2dad 100644 --- a/lib/librte_cmdline/cmdline_rdline.h +++ b/lib/librte_cmdline/cmdline_rdline.h @@ -93,7 +93,7 @@ extern "C" { #endif /* configuration */ -#define RDLINE_BUF_SIZE 256 +#define RDLINE_BUF_SIZE 512 #define RDLINE_PROMPT_SIZE 32 #define RDLINE_VT100_BUF_SIZE 8 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ -- 2.1.4
[dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer
From: Nelio Laranjeiro For RETA query/update with a table of 512 entries, buffers are too small to handle the request. Signed-off-by: Nelio Laranjeiro --- lib/librte_cmdline/cmdline_parse.h| 2 +- lib/librte_cmdline/cmdline_parse_string.h | 2 +- lib/librte_cmdline/cmdline_rdline.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/librte_cmdline/cmdline_parse.h b/lib/librte_cmdline/cmdline_parse.h index 4b25c45..89b28b1 100644 --- a/lib/librte_cmdline/cmdline_parse.h +++ b/lib/librte_cmdline/cmdline_parse.h @@ -81,7 +81,7 @@ extern "C" { #define CMDLINE_PARSE_COMPLETED_BUFFER 2 /* maximum buffer size for parsed result */ -#define CMDLINE_PARSE_RESULT_BUFSIZE 8192 +#define CMDLINE_PARSE_RESULT_BUFSIZE 65536 /** * Stores a pointer to the ops struct, and the offset: the place to diff --git a/lib/librte_cmdline/cmdline_parse_string.h b/lib/librte_cmdline/cmdline_parse_string.h index c205622..61e0627 100644 --- a/lib/librte_cmdline/cmdline_parse_string.h +++ b/lib/librte_cmdline/cmdline_parse_string.h @@ -66,7 +66,7 @@ extern "C" { #endif /* size of a parsed string */ -#define STR_TOKEN_SIZE 128 +#define STR_TOKEN_SIZE 8192 typedef char cmdline_fixed_string_t[STR_TOKEN_SIZE]; diff --git a/lib/librte_cmdline/cmdline_rdline.h b/lib/librte_cmdline/cmdline_rdline.h index b9aad9b..07f8faa 100644 --- a/lib/librte_cmdline/cmdline_rdline.h +++ b/lib/librte_cmdline/cmdline_rdline.h @@ -93,7 +93,7 @@ extern "C" { #endif /* configuration */ -#define RDLINE_BUF_SIZE 256 +#define RDLINE_BUF_SIZE 16384 #define RDLINE_PROMPT_SIZE 32 #define RDLINE_VT100_BUF_SIZE 8 #define RDLINE_HISTORY_BUF_SIZE BUFSIZ -- 2.1.0