From: "Govindharajan, Hariprasad" <hariprasad.govindharajan@intel.com> To: "Burakov, Anatoly" <anatoly.burakov@intel.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com> Cc: "dev@dpdk.org" <dev@dpdk.org>, "Yigit, Ferruh" <ferruh.yigit@intel.com>, "stephen@networkplumber.org" <stephen@networkplumber.org>, "david.marchand@redhat.com" <david.marchand@redhat.com> Subject: Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option Date: Tue, 11 Feb 2020 15:37:48 +0000 Message-ID: <BYAPR11MB2648E07EEAC06056B137595384180@BYAPR11MB2648.namprd11.prod.outlook.com> (raw) In-Reply-To: <0e183b56-2dce-a525-d6be-06a58d2e45af@intel.com> > -----Original Message----- > From: Burakov, Anatoly <anatoly.burakov@intel.com> > Sent: Tuesday, February 11, 2020 12:01 PM > To: Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; Lu, > Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; > Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John > <john.mcnamara@intel.com>; Kovacevic, Marko > <marko.kovacevic@intel.com> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; > stephen@networkplumber.org; david.marchand@redhat.com > Subject: Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option > > On 10-Feb-20 5:19 PM, Hariprasad Govindharajan wrote: > > In current version, we are setting the ports using portmask. With > > portmask, we can use only upto 64 ports. This portlist option enables > > the user to use more than 64 ports. > > Now we can specify the ports in 2 different ways > > - Using portmask (-p [0x]nnn): mask must be in hex format > > - Using portlist in the following format > > --portlist <p1>[-p2][,p3[-p4],...] > > > > --portmask 0x2 is same as --portlist 1 > > --portmask 0x3 is same as --portlist 0-1 > > > > Signed-off-by: Hariprasad Govindharajan > > <hariprasad.govindharajan@intel.com> > > --- > > v7: > > moved the port validation outside the parser function. > > added meaningful comments describing the new functionality. > > renamed the variables with meaningful names > > > > v6: > > optimized the code to check for duplicates > > > > v5: > > added a check to validate the ports available before setting them. > > also added comments in the testpmd file for the new function > > > > v4: > > the parser is modified so that we don't ues 2 arrays to convert the > > listed port values > > > > v3: > > squashed the 2 patches and made it 1 patch with changes only in > > testpmd. Also working on optmizing the parser > > > > v2: > > moved the parser function to testpmd > > --- > > app/test-pmd/config.c | 114 > ++++++++++++++++++++++++++++++++++ > > app/test-pmd/parameters.c | 5 ++ > > app/test-pmd/testpmd.h | 3 + > > doc/guides/testpmd_app_ug/run_app.rst | 7 +++ > > 4 files changed, 129 insertions(+) > > > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index > > 9669cbd..962984b 100644 > > --- a/app/test-pmd/config.c > > +++ b/app/test-pmd/config.c > > @@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist, > unsigned int nb_pt) > > } > > } > > > > +/** > > + * Parse the user input and obtain the list of forwarding ports > > + * > > + * @param[in] list > > + * String containing the user input. User can specify > > + * in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6. > > + * For example, if the user wants to use all the available > > + * 4 ports in his system, then the input can be 0-3 or 0,1,2,3. > > + * If the user wants to use only the ports 1,2 then the input > > + * is 1,2. > > + * valid characters are '-' and ',' > > + * invalid chars like '.' or '#' will result in > > + * EAL: Error - exiting with code: 1 > > + * Cause: Invalid fwd port list > > + * @param[out] values > > + * This array will be filled with a list of port IDs > > + * based on the user input > > + * Note that duplicate entries are discarded and only the first > > + * count entries in this array are port IDs and all the rest > > + * will contain default values > > + * @param[in] maxsize > > + * This parameter denotes 2 things > > + * 1) Size of the values array > > I believe you meant "number", not "size". [Govindharajan, Hariprasad] Nope. Here I meant to say the maximum size of the values array. > > > + * 2) Maximum value of each element in the values array > > + * @return > > + * -On success, returns total count of port IDs > > + * -On failure, returns -1. > > + */ > > +static int > > +parse_port_list(const char *list, unsigned int *values, int maxsize) > > +{ > > + int count = 0; > > + char *end = NULL; > > + int min, max; > > + int value, i; > > + unsigned int marked[maxsize]; > > + > > + for (i = 0; i < maxsize; i++) > > + marked[i] = 0; > > Wouldn't marked[maxsize] = {0}; work the same? [Govindharajan, Hariprasad] Nope. For that to work, the array size should be a constant. Here it is a variable. > > > + > > + if (list == NULL || values == NULL || maxsize < 0) > > + return -1; > > You're checking if maxsize can be negative. First of all, you've already > allocated the array with negative size by this time (the "marked[maxsize]" > one), second, why allow negative values at all? Why not just make it > unsigned? > > > + > > + /* Remove all blank characters ahead */ > > + while (isblank(*list)) > > + list++; > > Why do it here when you do this first thing in the do..while loop anyway? [Govindharajan, Hariprasad] Yes. Removed. > > > + > > + min = maxsize; > > You're overwriting this value regardless. Why not 0? If you want to know for > sure that the value either has or has not been modified, the conventional > way to do this is to use INT_MAX from <limits.h>. > > > + > > + do { > > + while (isblank(*list)) > > + list++; > > I have a suspicion that isblank() will not return 'true' on '\0' so there's > probably a buffer overrun here, if you try to dereference *list while going > past '\0'. [Govindharajan, Hariprasad] Corrected > > > + if (*list == '\0') > > + return -1; > > + errno = 0; > > + value = strtol(list, &end, 10); > > + if (errno || end == NULL) > > + return -1; > > + if (value < 0 || value >= maxsize) > > + return -1; > > + while (isblank(*end)) > > + end++; > > + if (*end == '-') { > > + min = value; > > + } else if ((*end == ',') || (*end == '\0')) { > > + max = value; > > + if (min == maxsize) > > + min = value; > > + for (i = min; i <= max; i++) { > > + if (count < maxsize) { > > + if (marked[i]) > > + continue; > > + values[count] = i; > > + marked[i] = 1; > > + count++; > > + } > > + } > > + min = maxsize; > > Probably clearer to reset both to zero or INT_MAX/INT_MIN? [Govindharajan, Hariprasad] done > > > + } else > > + return -1; > > + list = end + 1; > > + } while (*end != '\0'); > > + > > + if (count == 0) > > + return -1; > > + return count; > > +} > > + > > +void > > +parse_fwd_portlist(const char *portlist) { > > + int portcount; > > + unsigned int portindex[RTE_MAX_ETHPORTS]; > > + int i, valid_port_count = 0; > > unsigned? [Govindharajan, Hariprasad] Changed. Initially I was comparing those 2 variables with a signed variable, So declared them as signed as well. > > > + > > + portcount = parse_port_list(portlist, portindex, > RTE_MAX_ETHPORTS); > > + if (portcount < 0) > > + rte_exit(EXIT_FAILURE, "Invalid fwd port list\n"); > > + > > + /* > > + * Here we verify the validity of the ports > > + * and thereby calculate the total number of > > + * valid ports > > + */ > > -- > Thanks, > Anatoly
next prev parent reply other threads:[~2020-02-11 15:37 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-27 10:30 [dpdk-dev] [PATCH 0/2] app/testpmd: add portlist option to the testpmd Hariprasad Govindharajan 2020-01-27 10:30 ` [dpdk-dev] [PATCH 1/2] " Hariprasad Govindharajan 2020-01-31 16:22 ` [dpdk-dev] [PATCH v2] " Hariprasad Govindharajan 2020-01-31 17:41 ` [dpdk-dev] [PATCH v3] " Hariprasad Govindharajan 2020-02-03 16:53 ` [dpdk-dev] [PATCH v4] " Hariprasad Govindharajan 2020-02-04 16:48 ` [dpdk-dev] [PATCH v5] " Hariprasad Govindharajan 2020-02-05 10:42 ` Ferruh Yigit 2020-02-06 15:03 ` [dpdk-dev] [PATCH v6] " Hariprasad Govindharajan 2020-02-10 17:19 ` [dpdk-dev] [PATCH v7] app/testpmd: add portlist option Hariprasad Govindharajan 2020-02-11 12:00 ` Burakov, Anatoly 2020-02-11 15:37 ` Govindharajan, Hariprasad [this message] 2020-02-11 15:52 ` [dpdk-dev] [PATCH v8] " Hariprasad Govindharajan 2020-02-11 16:51 ` Burakov, Anatoly 2020-02-12 10:25 ` Govindharajan, Hariprasad 2020-02-12 10:29 ` [dpdk-dev] [PATCH v9] " Hariprasad Govindharajan 2020-02-12 13:03 ` Burakov, Anatoly 2020-02-12 13:59 ` Ferruh Yigit 2020-02-12 17:27 ` Lipiec, Herakliusz 2020-02-13 11:31 ` Ferruh Yigit 2020-01-27 10:30 ` [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input Hariprasad Govindharajan 2020-01-28 17:35 ` Ferruh Yigit 2020-01-29 17:44 ` David Marchand 2020-01-29 18:07 ` Ferruh Yigit 2020-01-29 19:19 ` Stephen Hemminger 2020-01-31 11:25 ` Govindharajan, Hariprasad 2020-01-31 14:42 ` Govindharajan, Hariprasad
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=BYAPR11MB2648E07EEAC06056B137595384180@BYAPR11MB2648.namprd11.prod.outlook.com \ --to=hariprasad.govindharajan@intel.com \ --cc=anatoly.burakov@intel.com \ --cc=bernard.iremonger@intel.com \ --cc=david.marchand@redhat.com \ --cc=dev@dpdk.org \ --cc=ferruh.yigit@intel.com \ --cc=jingjing.wu@intel.com \ --cc=john.mcnamara@intel.com \ --cc=marko.kovacevic@intel.com \ --cc=stephen@networkplumber.org \ --cc=wenzhuo.lu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git