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

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

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