[dpdk-dev,v2,1/3] cmdline: increase command line buffer

  Allow long command lines in testpmd (like flow director with IPv6, ...).

Signed-off-by: John McNamara <john.mcnamara@intel.com>
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 -----
 lib/librte_cmdline/cmdline_rdline.h  | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)
  

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 <john.mcnamara@intel.com>
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>   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 -
  

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 <john.mcnamara@intel.com>
> >Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> >---
> >  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
  

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 <john.mcnamara@intel.com>
>>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>> ---
>>>   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 -
  

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
  

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,
  

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Friday, February 26, 2016 3:17 PM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: Olivier MATZ <olivier.matz@6wind.com>; Panu Matilainen
> <pmatilai@redhat.com>; dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>;
> thomas.monjalon@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> olgas@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 <john.mcnamara@intel.com>