From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id BF33311C5 for ; Mon, 18 Jan 2016 15:38:50 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1aLAyM-0002VC-6a; Mon, 18 Jan 2016 15:39:58 +0100 To: Panu Matilainen , =?UTF-8?Q?N=c3=a9lio_Laranjeiro?= , john.mcnamara@intel.com References: <1452090774-10650-1-git-send-email-nelio.laranjeiro@6wind.com> <1452595749-11297-1-git-send-email-nelio.laranjeiro@6wind.com> <1452595749-11297-2-git-send-email-nelio.laranjeiro@6wind.com> <5694F58F.1040105@redhat.com> <20160115084439.GW13678@autoinstall.dev.6wind.com> <5698B51B.60603@redhat.com> From: Olivier MATZ X-Enigmail-Draft-Status: N1110 Message-ID: <569CF8F3.2020908@6wind.com> Date: Mon, 18 Jan 2016 15:38:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.3.0 MIME-Version: 1.0 In-Reply-To: <5698B51B.60603@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 1/3] cmdline: increase command line buffer X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jan 2016 14:38:50 -0000 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