From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 08A0FA0A03; Tue, 19 Jan 2021 08:45:38 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 70BDF140D07; Tue, 19 Jan 2021 08:45:38 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id CCB63140D06 for ; Tue, 19 Jan 2021 08:45:35 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3D28E7F4AC; Tue, 19 Jan 2021 10:45:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3D28E7F4AC DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1611042335; bh=D/R0QvJuNJiP49jswDXXZye1Xoza4tPXTabqGOChPd0=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=S9yJXRvwruXjbVIxutLBQO6vfODQrZTjzHNrlsQflcNBiXxpeWViYmuFmK8YZxdsI KOKIs90ZytvvqpWvdqPU4ZiCezZCIEZlCdEs9SZ2JDbnYh7mJ/0BiCJnvXExrVf745 lRu4QDhY8qdFmS9o+iw7EHlClOayOaUyLxjG9zXs= To: Xueming Li , Thomas Monjalon , Ferruh Yigit , Olivier Matz Cc: dev@dpdk.org, Viacheslav Ovsiienko , Asaf Penso References: <1610968623-31345-1-git-send-email-xuemingl@nvidia.com> <1610968623-31345-3-git-send-email-xuemingl@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Tue, 19 Jan 2021 10:45:35 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <1610968623-31345-3-git-send-email-xuemingl@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 2/9] ethdev: support representor port list X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/18/21 2:16 PM, Xueming Li wrote: > To support extended representor syntax, this patch extends the > representor list parsing to support for representor port range in > devargs, examples: > representor=[1,2,3] - single list > representor=[1,3-5,7,9-11] - list with singles and ranges > > Signed-off-by: Xueming Li > Acked-by: Viacheslav Ovsiienko See below > --- > lib/librte_ethdev/ethdev_private.c | 105 ++++++++++++++--------------- > lib/librte_ethdev/ethdev_private.h | 3 - > lib/librte_ethdev/rte_class_eth.c | 4 +- > lib/librte_ethdev/rte_ethdev.c | 5 +- > 4 files changed, 54 insertions(+), 63 deletions(-) > > diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c > index c1a411dba4..12bcc7e98d 100644 > --- a/lib/librte_ethdev/ethdev_private.c > +++ b/lib/librte_ethdev/ethdev_private.c > @@ -38,77 +38,71 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp, > return NULL; > } > > -int > -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback, > - void *data) > +static int > +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list, > + const uint16_t max_list, uint16_t val) > { > - char *str_start; > - int state; > - int result; > - > - if (*str != '[') > - /* Single element, not a list */ > - return callback(str, data); > - > - /* Sanity check, then strip the brackets */ > - str_start = &str[strlen(str) - 1]; > - if (*str_start != ']') { > - RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str); > - return -EINVAL; > - } > - str++; > - *str_start = '\0'; > + uint16_t i; > > - /* Process list elements */ > - state = 0; > - while (1) { > - if (state == 0) { > - if (*str == '\0') > - break; > - if (*str != ',') { > - str_start = str; > - state = 1; > - } > - } else if (state == 1) { > - if (*str == ',' || *str == '\0') { > - if (str > str_start) { > - /* Non-empty string fragment */ > - *str = '\0'; > - result = callback(str_start, data); > - if (result < 0) > - return result; > - } > - state = 0; > - } > - } > - str++; > + if (*len_list >= max_list) > + return -1; If current length is equal to max, but added value is already is in the list, it should not be an error. So, these two lines should be moved after below for loop. > + for (i = 0; i < *len_list; i++) { > + if (list[i] == val) > + return 0; > } > + list[(*len_list)++] = val; > return 0; > } > > -static int > +static char * > rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list, > const uint16_t max_list) > { > uint16_t lo, hi, val; > int result; > + char *pos = str; > > result = sscanf(str, "%hu-%hu", &lo, &hi); > if (result == 1) { > - if (*len_list >= max_list) > - return -ENOMEM; > - list[(*len_list)++] = lo; > + if (rte_eth_devargs_enlist(list, len_list, max_list, lo) != 0) > + return NULL; > } else if (result == 2) { > - if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS) Strictly speaking removal of comparision vs RTE_MAX_ETHPORTS is a separate logical change with a separate motivation. > - return -EINVAL; > + if (lo >= hi) I'd remove '=' here. It should not be a problem and handed perfectly by below code. I see no point to deny 3-3 range which is an equivalent for just 3. It could be convenient in some cases. > + return NULL; > for (val = lo; val <= hi; val++) { > - if (*len_list >= max_list) > - return -ENOMEM; > - list[(*len_list)++] = val; > + if (rte_eth_devargs_enlist(list, len_list, max_list, > + val) != 0) > + return NULL; > } > } else > - return -EINVAL; > - return 0; > + return NULL; > + while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-')) *post != '\0' is a bit better looking at subsequent comparisons. Yes, it is just style. Up to you. > + pos++; It looks too fragile. May I suggest to use %n in above scanf to be able to skip only parsed characters. > + return pos; > +} > + > +static char * > +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list, > + const uint16_t max_list) > +{ > + char *pos = str; > + > + if (*pos == '[') > + pos++; > + while (1) { > + pos = rte_eth_devargs_process_range(pos, list, len_list, > + max_list); > + if (pos == NULL) > + return NULL; > + if (*pos != ',') /* end of list */ > + break; > + pos++; > + } > + if (*str == '[' && *pos != ']') > + return NULL; > + if (*pos == ']') > + pos++; > + return pos; > } > > /* > @@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data) > struct rte_eth_devargs *eth_da = data; > > eth_da->type = RTE_ETH_REPRESENTOR_VF; > - return rte_eth_devargs_process_range(str, eth_da->representor_ports, > + str = rte_eth_devargs_process_list(str, eth_da->representor_ports, > ð_da->nb_representor_ports, RTE_MAX_ETHPORTS); Not directly related to the patch, but I dislike RTE_MAX_ETHPORTS above. RTE_DIM(eth_da->representor_ports) would be more readable. > + if (str == NULL) > + RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str); > + return str == NULL ? -1 : 0; > } [snip]