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

[Olivier MATZ <olivier.matz@6wind.com>]

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