Message ID | 1452595749-11297-2-git-send-email-nelio.laranjeiro@6wind.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Thomas Monjalon |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id B2BE48E6A; Tue, 12 Jan 2016 11:49:45 +0100 (CET) Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 3B8EC8E65 for <dev@dpdk.org>; Tue, 12 Jan 2016 11:49:44 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id u188so250227563wmu.1 for <dev@dpdk.org>; Tue, 12 Jan 2016 02:49:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=MQoK1MYT1hoITgwLh4zB9hc2SHg52N713PONE6sxj2Q=; b=fcC6Bqo8Q/6IGsdvoIWsttjpnxIqLUVFC7C9pu1qA2y6fYlJHES46fXKqR6N1xHxDm 4Q6qcW6YQKmBMB7o7BsR9Dj3FQD5I4wIqaveIEn8HIe0Aya4EKfVxp9gpWMh0fI9Xzwh M2F7B+RzSXzNExPk3Q2kEGfDLEFXwvActriAPY5Lm1gtlgIXF5aZ5ZA8VzxcgykzUDUF K/GqdpBEioeqz/gtss4Rsm9VfiI6XdGq97bUej3KBH/TXVqIDldKiiKzpKS0gaT2kzcy S2Ot7qL1ZK/QzgMiMUgPUkx6e4E5NXyCX+RzWy1dBet1fH51QfESoq5Z7hnn3chMHkUZ 4isw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=MQoK1MYT1hoITgwLh4zB9hc2SHg52N713PONE6sxj2Q=; b=Zo5a3HdxRZvOpkjqsP4zk/gD9sv5a42v12mpgWSDNYiqr8nnC8xikvqNVOtpqDvvhu 1gK/I13aUTRDBIEz1K+zjheDQyUPr8uzFxmDN6Uw4CqNub27671GBY5ONa50UFSID2V4 /1bX3IeQNXyVRfmhDmzh/b1aROv8NMfPZmkapdMM/BM29gY500KPHvVXdZHi68RpNTMl I1004xJoYXuVjWv2Qcdr/tqRrT2mDpFZTrB50Ecb7HFT08xGeIUMvFv3iCHnId7wc4he y8O+vYoAm4VAcZYuFVhoEhGvGTLIHi8pvft2wjX9PldUsuQ/lreTT03FYJ36ZuKSa96x JS4w== X-Gm-Message-State: ALoCoQkbTJIRvXpCfwpWGVNEjZYzLh3uYgdvoAnN795wR6/OoxPGnM+xmdujJqR0Wh0kS3YmqWO6S11hbC9eOxxGd/VSzqhZxg== X-Received: by 10.28.50.193 with SMTP id y184mr9014542wmy.103.1452595784014; Tue, 12 Jan 2016 02:49:44 -0800 (PST) Received: from ping.vm.6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id v191sm15884657wme.1.2016.01.12.02.49.42 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 12 Jan 2016 02:49:43 -0800 (PST) From: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> To: dev@dpdk.org Date: Tue, 12 Jan 2016 11:49:07 +0100 Message-Id: <1452595749-11297-2-git-send-email-nelio.laranjeiro@6wind.com> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1452595749-11297-1-git-send-email-nelio.laranjeiro@6wind.com> References: <1452090774-10650-1-git-send-email-nelio.laranjeiro@6wind.com> <1452595749-11297-1-git-send-email-nelio.laranjeiro@6wind.com> Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Nélio Laranjeiro
Jan. 12, 2016, 10:49 a.m. UTC
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(-)
Comments
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>
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