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

[Panu Matilainen <pmatilai@redhat.com>]

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 -