From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f52.google.com (mail-pg0-f52.google.com [74.125.83.52]) by dpdk.org (Postfix) with ESMTP id A7A4E8CF4 for ; Mon, 21 Aug 2017 11:35:13 +0200 (CEST) Received: by mail-pg0-f52.google.com with SMTP id t3so68824703pgt.0 for ; Mon, 21 Aug 2017 02:35:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fridaylinux-org.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=00w4536xx35BME1qrsPKx3lG9sgRqj4ZmGXzlqHEy6Q=; b=MMawrc/la+i0XAhuLGK7ZP4kzWKhhghB46OkcatELcHz8yM0If30YALMR1EZN3/jPr mnxrLrvaTZe9aj/8fAUiiWCEXUzVOrvVvhUnvOmnepdgL7TmiYD3dODccKxC/xTaB+zv 1glrZmYjm7xKmqOjXaCT30DHgjkqELbM+KIXzlpgYU4isHS3tV8zMwad6q3Ix1WEr/6w PLoiO8H+cTKcuUxceLXTFyiC3/6poDFJlVUQ18DwgXyJuyLmHcaOghQIy3aVrYgrEIUm H/SDFCiIAJuHRSQ/WZQKFX8o9ODDdwS2aDXHYx/QPcRXm9m3prtcniyTbdDuYeSsKN5Q c1MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=00w4536xx35BME1qrsPKx3lG9sgRqj4ZmGXzlqHEy6Q=; b=WoZ2oXE4TBD6KN2EYHJKSuElqk6rhHcB6V63VVloXoz7N8qpgAnnRjTbppcLR9U0qa H8WJ18pPrHb3PE1GNW3JFPcIy5SpFcpVpcmV2ihQqbRqczIoVt2l+PyYkkT8FpvDChIl bN8cvdg3RJyZNoIq1dYo+oMETjPk8POsQdKpSwFHntiIw++3FGxkVA6ss3RfOsXbB4mL qjZO3ctCpoMbYtm7MrEHHoWDfq3lh1W37tP58bLlyjOQyDoaHVl2feV32UlMfKYbSfqj bTWfs9PnSCYAf7HXVP9JwrLkhEaxrT02ITyjL/J+hsGSLB1koM6734obriKEkxyIljg9 GBMA== X-Gm-Message-State: AHYfb5jtmrW2Z+PUVHmcn+qBESGjnXrsBcOMBL9jfEsOicPfKC2XDuLA pZKwR4LjfddXpSnv X-Received: by 10.84.217.206 with SMTP id d14mr12147365plj.319.1503308112800; Mon, 21 Aug 2017 02:35:12 -0700 (PDT) Received: from localhost.localdomain ([45.63.61.64]) by smtp.gmail.com with ESMTPSA id 69sm23355087pfh.186.2017.08.21.02.35.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 21 Aug 2017 02:35:11 -0700 (PDT) From: Yuanhan Liu To: Adrien Mazarguil Cc: Olivier Matz , dpdk stable , Yuanhan Liu Date: Mon, 21 Aug 2017 17:30:39 +0800 Message-Id: <1503307878-16728-25-git-send-email-yliu@fridaylinux.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1503307878-16728-1-git-send-email-yliu@fridaylinux.org> References: <1503307878-16728-1-git-send-email-yliu@fridaylinux.org> Subject: [dpdk-stable] patch 'cmdline: fix dynamic tokens interface' has been queued to stable release 17.05.2 X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Aug 2017 09:35:14 -0000 Hi, FYI, your patch has been queued to stable release 17.05.2 Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. It will be pushed if I get no objections before 08/24/17. So please shout if anyone has objections. Thanks. --yliu --- >>From d9663e500543c050478e9bfaef9d48f4854c8125 Mon Sep 17 00:00:00 2001 From: Adrien Mazarguil Date: Mon, 10 Jul 2017 14:09:35 +0200 Subject: [PATCH] cmdline: fix dynamic tokens interface [ upstream commit a9510e11ad2de08ba9a56b5f8fca4745fe6c72ca ] Support for dynamic tokens was added in order to implement the flow command in testpmd, for which static tokens were not versatile enough due to the large number of possible parameter combinations. However, due to its reliance on a temporary array to store dynamic tokens, this interface suffers from various limitations that need to be addressed in order to implement more commands in the future: - The maximum number of dynamic tokens is determined at compilation time (CMDLINE_PARSE_DYNAMIC_TOKENS). The larger this value, the more stack space is wasted (one pointer per potential token, i.e. 1kB of stack space on 64-bit architectures with the default value). - This temporary array is actually a cache in which entries already present are not regenerated. This behavior is not documented, which makes dynamic tokens practically unusable by applications as they do not know which token is current. - The cache does not really reduce the number of function calls needed to retrieve tokens, it was mainly deemed useful to provide context about other tokens to the generator callback. - Like testpmd, most users will likely use repeated pointers to a fixed token header structure (cmdline_token_hdr_t), with internal context-aware callbacks that do not need to look at other entries; knowing the index of the current token is enough. Getting rid of the temporary array and properly documenting usage of the token generator callback greatly simplifies this interface. Fixes: 4fffc05a2b2c ("cmdline: support dynamic tokens") Fixes: 19c90af6285c ("app/testpmd: add flow command") Signed-off-by: Adrien Mazarguil Acked-by: Olivier Matz --- app/test-pmd/cmdline_flow.c | 15 ++------- lib/librte_cmdline/cmdline_parse.c | 63 +++++++++++--------------------------- lib/librte_cmdline/cmdline_parse.h | 50 ++++++++++++++++++++++++------ 3 files changed, 60 insertions(+), 68 deletions(-) diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index 0fd69f9..dfff8be 100644 --- a/app/test-pmd/cmdline_flow.c +++ b/app/test-pmd/cmdline_flow.c @@ -220,7 +220,6 @@ struct context { enum index prev; /**< Index of the last token seen. */ int next_num; /**< Number of entries in next[]. */ int args_num; /**< Number of entries in args[]. */ - uint32_t reparse:1; /**< Start over from the beginning. */ uint32_t eol:1; /**< EOL has been detected. */ uint32_t last:1; /**< No more arguments. */ uint16_t port; /**< Current port ID (for completions). */ @@ -2534,7 +2533,6 @@ cmd_flow_context_init(struct context *ctx) ctx->prev = ZERO; ctx->next_num = 0; ctx->args_num = 0; - ctx->reparse = 0; ctx->eol = 0; ctx->last = 0; ctx->port = 0; @@ -2555,9 +2553,6 @@ cmd_flow_parse(cmdline_parse_token_hdr_t *hdr, const char *src, void *result, int i; (void)hdr; - /* Restart as requested. */ - if (ctx->reparse) - cmd_flow_context_init(ctx); token = &token_list[ctx->curr]; /* Check argument length. */ ctx->eol = 0; @@ -2633,8 +2628,6 @@ cmd_flow_complete_get_nb(cmdline_parse_token_hdr_t *hdr) int i; (void)hdr; - /* Tell cmd_flow_parse() that context must be reinitialized. */ - ctx->reparse = 1; /* Count number of tokens in current list. */ if (ctx->next_num) list = ctx->next[ctx->next_num - 1]; @@ -2668,8 +2661,6 @@ cmd_flow_complete_get_elt(cmdline_parse_token_hdr_t *hdr, int index, int i; (void)hdr; - /* Tell cmd_flow_parse() that context must be reinitialized. */ - ctx->reparse = 1; /* Count number of tokens in current list. */ if (ctx->next_num) list = ctx->next[ctx->next_num - 1]; @@ -2704,8 +2695,6 @@ cmd_flow_get_help(cmdline_parse_token_hdr_t *hdr, char *dst, unsigned int size) const struct token *token = &token_list[ctx->prev]; (void)hdr; - /* Tell cmd_flow_parse() that context must be reinitialized. */ - ctx->reparse = 1; if (!size) return -1; /* Set token type and update global help with details. */ @@ -2731,12 +2720,12 @@ static struct cmdline_token_hdr cmd_flow_token_hdr = { /** Populate the next dynamic token. */ static void cmd_flow_tok(cmdline_parse_token_hdr_t **hdr, - cmdline_parse_token_hdr_t *(*hdrs)[]) + cmdline_parse_token_hdr_t **hdr_inst) { struct context *ctx = &cmd_flow_context; /* Always reinitialize context before requesting the first token. */ - if (!(hdr - *hdrs)) + if (!(hdr_inst - cmd_flow.tokens)) cmd_flow_context_init(ctx); /* Return NULL when no more tokens are expected. */ if (!ctx->next_num && ctx->curr) { diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c index 67e452d..56491ea 100644 --- a/lib/librte_cmdline/cmdline_parse.c +++ b/lib/librte_cmdline/cmdline_parse.c @@ -141,28 +141,17 @@ nb_common_chars(const char * s1, const char * s2) /** Retrieve either static or dynamic token at a given index. */ static cmdline_parse_token_hdr_t * -get_token(cmdline_parse_inst_t *inst, - unsigned int index, - cmdline_parse_token_hdr_t - *(*dyn_tokens)[CMDLINE_PARSE_DYNAMIC_TOKENS]) +get_token(cmdline_parse_inst_t *inst, unsigned int index) { + cmdline_parse_token_hdr_t *token_p; + /* check presence of static tokens first */ if (inst->tokens[0] || !inst->f) return inst->tokens[index]; - /* for dynamic tokens, make sure index does not overflow */ - if (index >= CMDLINE_PARSE_DYNAMIC_TOKENS - 1) - return NULL; - /* in case token is already generated, return it */ - if ((*dyn_tokens)[index]) - return (*dyn_tokens)[index]; - /* generate token */ - inst->f(&(*dyn_tokens)[index], NULL, dyn_tokens); - /* return immediately if there are no more tokens to expect */ - if (!(*dyn_tokens)[index]) - return NULL; - /* terminate list with a NULL entry */ - (*dyn_tokens)[index + 1] = NULL; - return (*dyn_tokens)[index]; + /* generate dynamic token */ + token_p = NULL; + inst->f(&token_p, NULL, &inst->tokens[index]); + return token_p; } /** @@ -172,22 +161,20 @@ get_token(cmdline_parse_inst_t *inst, */ static int match_inst(cmdline_parse_inst_t *inst, const char *buf, - unsigned int nb_match_token, void *resbuf, unsigned resbuf_size, - cmdline_parse_token_hdr_t - *(*dyn_tokens)[CMDLINE_PARSE_DYNAMIC_TOKENS]) + unsigned int nb_match_token, void *resbuf, unsigned resbuf_size) { - unsigned int token_num=0; cmdline_parse_token_hdr_t * token_p; unsigned int i=0; int n = 0; struct cmdline_token_hdr token_hdr; - token_p = get_token(inst, token_num, dyn_tokens); - if (token_p) + /* check if we match all tokens of inst */ + while (!nb_match_token || i < nb_match_token) { + token_p = get_token(inst, i); + if (!token_p) + break; memcpy(&token_hdr, token_p, sizeof(token_hdr)); - /* check if we match all tokens of inst */ - while (token_p && (!nb_match_token || i 0) /* we matched at least one token */ err = CMDLINE_PARSE_BAD_ARGS; @@ -365,7 +345,6 @@ cmdline_parse(struct cmdline *cl, const char * buf) inst_num ++; inst = ctx[inst_num]; - dyn_tokens[0] = NULL; } /* call func */ @@ -392,7 +371,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, cmdline_parse_token_hdr_t *token_p; struct cmdline_token_hdr token_hdr; char tmpbuf[CMDLINE_BUFFER_SIZE], comp_buf[CMDLINE_BUFFER_SIZE]; - cmdline_parse_token_hdr_t *dyn_tokens[CMDLINE_PARSE_DYNAMIC_TOKENS]; unsigned int partial_tok_len; int comp_len = -1; int tmp_len = -1; @@ -432,16 +410,14 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, nb_non_completable = 0; inst = ctx[inst_num]; - dyn_tokens[0] = NULL; while (inst) { /* parse the first tokens of the inst */ if (nb_token && - match_inst(inst, buf, nb_token, NULL, 0, - &dyn_tokens)) + match_inst(inst, buf, nb_token, NULL, 0)) goto next; debug_printf("instruction match\n"); - token_p = get_token(inst, nb_token, &dyn_tokens); + token_p = get_token(inst, nb_token); if (token_p) memcpy(&token_hdr, token_p, sizeof(token_hdr)); @@ -495,7 +471,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, debug_printf("next\n"); inst_num ++; inst = ctx[inst_num]; - dyn_tokens[0] = NULL; } debug_printf("total choices %d for this completion\n", @@ -528,16 +503,15 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, inst_num = 0; inst = ctx[inst_num]; - dyn_tokens[0] = NULL; while (inst) { /* we need to redo it */ inst = ctx[inst_num]; if (nb_token && - match_inst(inst, buf, nb_token, NULL, 0, &dyn_tokens)) + match_inst(inst, buf, nb_token, NULL, 0)) goto next2; - token_p = get_token(inst, nb_token, &dyn_tokens); + token_p = get_token(inst, nb_token); if (token_p) memcpy(&token_hdr, token_p, sizeof(token_hdr)); @@ -610,7 +584,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, next2: inst_num ++; inst = ctx[inst_num]; - dyn_tokens[0] = NULL; } return 0; } diff --git a/lib/librte_cmdline/cmdline_parse.h b/lib/librte_cmdline/cmdline_parse.h index 65b18d4..13e086f 100644 --- a/lib/librte_cmdline/cmdline_parse.h +++ b/lib/librte_cmdline/cmdline_parse.h @@ -83,9 +83,6 @@ extern "C" { /* maximum buffer size for parsed result */ #define CMDLINE_PARSE_RESULT_BUFSIZE 8192 -/* maximum number of dynamic tokens */ -#define CMDLINE_PARSE_DYNAMIC_TOKENS 128 - /** * Stores a pointer to the ops struct, and the offset: the place to * write the parsed result in the destination structure. @@ -137,20 +134,53 @@ struct cmdline; * When no tokens are defined (tokens[0] == NULL), they are retrieved * dynamically by calling f() as follows: * - * f((struct cmdline_token_hdr **)&token_hdr, - * NULL, - * (struct cmdline_token_hdr *[])tokens)); + * @code + * + * f((struct cmdline_token_hdr **)&token_p, + * NULL, + * (struct cmdline_token_hdr **)&inst->tokens[num]); + * + * @endcode * * The address of the resulting token is expected at the location pointed by * the first argument. Can be set to NULL to end the list. * * The cmdline argument (struct cmdline *) is always NULL. * - * The last argument points to the NULL-terminated list of dynamic tokens - * defined so far. Since token_hdr points to an index of that list, the - * current index can be derived as follows: + * The last argument points to the inst->tokens[] entry to retrieve, which + * is not necessarily inside allocated memory and should neither be read nor + * written. Its sole purpose is to deduce the token entry index of interest + * as described in the example below. + * + * Note about constraints: + * + * - Only the address of these tokens is dynamic, their storage should be + * static like normal tokens. + * - Dynamic token lists that need to maintain an internal context (e.g. in + * order to determine the next token) must store it statically also. This + * context must be reinitialized when the first token is requested, that + * is, when &inst->tokens[0] is provided as the third argument. + * - Dynamic token lists must be NULL-terminated to generate usable + * commands. + * + * @code + * + * // Assuming first and third arguments are respectively named "token_p" + * // and "token": + * + * int index = token - inst->tokens; + * + * if (!index) { + * [...] // Clean up internal context if any. + * } + * [...] // Then set up dyn_token according to index. + * + * if (no_more_tokens) + * *token_p = NULL; + * else + * *token_p = &dyn_token; * - * int index = token_hdr - &(*tokens)[0]; + * @endcode */ struct cmdline_inst { /* f(parsed_struct, data) */ -- 2.7.4