From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id DF6D25B34 for ; Fri, 30 Mar 2018 17:53:49 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id x82so17372132wmg.1 for ; Fri, 30 Mar 2018 08:53:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=DNNai7dN0xZKvzVIdBPewNOUlpUY6rEYye/6iBEYFVY=; b=nZbBAEJzBD0O06vnZI7IgbrD2PrTelCDvyk64hGnHdfXw1ee/JnDom+ucUJtzKwwPR 863jFR3rqKx78wU7YlDcjnFCX24Brbt+jWKFKeYeDGLVay8sQ2bMiezwKwMa8SxGafRo J8lJ834I/4+58RMBoZIxrEmliRbyER1Q8re5ISneBoMTv7qqCWeRHl53Skb7Lc5JHoYo ZjKJGhqmgwZpfn2vSVFmtGv0+2mABB0Pd+fFPfOX8/W1qB9U5F1lRdLbuwVAZaEV9W7V ovy94IfRccnrbK9g0ezp5iISzEE6wLzdu6SjxO7fyjoSZU5arUBl2sFLzxo4YFZSxZuu pcFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=DNNai7dN0xZKvzVIdBPewNOUlpUY6rEYye/6iBEYFVY=; b=ta3aFhKPhXoff2q7jgmQMJwY77FCWe9Impbxiu9C07L/p84rBN/Lp6j/e6Eb3ZrtXE 5b6RRbwRmMVC+0EWcx/s2gm+irpEfjEkPGDJU5bdxJOBFApi9N3x8pCwIrAqGnUbqF4I X2HLf0ChXNPPriyJYwfwg9j9pMacqVe88frRabMVjnuEJFd3qtNQ50yxJt4FKVNuuGq9 MbMn2SdhRFswmkoVXg9RsmsdX7I9+JxipEoIsAJrOVgnsgzF66XhZh5Ol3GrhH9vFleF m+al/pxaM/1cGweJtMfAOKcE9iJL8VOvpRFjOBwornyMYtXFu/LTYBd6q8aQ9VNGrTeU Ny7w== X-Gm-Message-State: AElRT7FgQKulEk4xpRQ6MLa7zWDhOHwOyTCPfMpQDM54gbuhT08V5GxA PadQ/8UE6fpsqbcYi4CM/XD7tw== X-Google-Smtp-Source: AIpwx49KPTZ7QDcxxpGyy/GJwOSEVuIAxatge2wQMXXBWhYmysK5iubKQjJkaXOipJrxxOXF6844gA== X-Received: by 10.28.239.18 with SMTP id n18mr2752350wmh.56.1522425229270; Fri, 30 Mar 2018 08:53:49 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id y144sm7277778wmd.47.2018.03.30.08.53.48 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 30 Mar 2018 08:53:48 -0700 (PDT) Date: Fri, 30 Mar 2018 17:53:35 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: "Wiles, Keith" Cc: "dev@dpdk.org" Message-ID: <20180330155335.3tnaczturi6gvnzl@bidouze.vm.6wind.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v4 09/20] eal/dev: implement device iteration initialization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Mar 2018 15:53:50 -0000 Hello Keith, On Fri, Mar 30, 2018 at 03:22:59PM +0000, Wiles, Keith wrote: > > > > On Mar 29, 2018, at 4:23 PM, Gaetan Rivet wrote: > > > > Parse a device description. > > Split this description in their relevant part for each layers. > > No dynamic allocation is performed. > > > > Signed-off-by: Gaetan Rivet > > --- > > lib/Makefile | 1 + > > lib/librte_eal/bsdapp/eal/Makefile | 1 + > > lib/librte_eal/common/eal_common_dev.c | 147 ++++++++++++++++++++++++++++++++ > > lib/librte_eal/common/include/rte_dev.h | 23 +++++ > > lib/librte_eal/linuxapp/eal/Makefile | 1 + > > lib/librte_eal/rte_eal_version.map | 1 + > > 6 files changed, 174 insertions(+) > > > > diff --git a/lib/Makefile b/lib/Makefile > > index fc7a55a37..1b17526f7 100644 > > --- a/lib/Makefile > > +++ b/lib/Makefile > > @@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk > > DIRS-y += librte_compat > > DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs > > DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal > > +DEPDIRS-librte_eal := librte_kvargs > > DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci > > DEPDIRS-librte_pci := librte_eal > > DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring > > diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile > > index 17ff1ac45..f6cea7fc2 100644 > > --- a/lib/librte_eal/bsdapp/eal/Makefile > > +++ b/lib/librte_eal/bsdapp/eal/Makefile > > @@ -18,6 +18,7 @@ CFLAGS += $(WERROR_FLAGS) -O3 > > LDLIBS += -lexecinfo > > LDLIBS += -lpthread > > LDLIBS += -lgcc_s > > +LDLIBS += -lrte_kvargs > > > > EXPORT_MAP := ../../rte_eal_version.map > > > > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c > > index cd071442f..1f6df2351 100644 > > --- a/lib/librte_eal/common/eal_common_dev.c > > +++ b/lib/librte_eal/common/eal_common_dev.c > > @@ -10,9 +10,12 @@ > > > > #include > > #include > > +#include > > #include > > #include > > #include > > +#include > > +#include > > #include > > > > #include "eal_private.h" > > @@ -207,3 +210,147 @@ rte_eal_hotplug_remove(const char *busname, const char *devname) > > rte_eal_devargs_remove(busname, devname); > > return ret; > > } > > + > > +static size_t > > +dev_layer_count(const char *s) > > +{ > > + size_t i = s ? 1 : 0; > > + > > + while (s != NULL && s[0] != '\0') { > > + i += s[0] == '/'; > > + s++; > > + } > > + return i; > > +} > > + > > +int __rte_experimental > > +rte_dev_iterator_init(struct rte_dev_iterator *it, > > + const char *devstr) > > +{ > > + struct { > > + const char *key; > > + const char *str; > > + struct rte_kvargs *kvlist; > > + } layers[] = { > > + { "bus=", NULL, NULL, }, > > + { "class=", NULL, NULL, }, > > + { "driver=", NULL, NULL, }, > > + }; > > + struct rte_kvargs_pair *kv = NULL; > > + struct rte_class *cls = NULL; > > + struct rte_bus *bus = NULL; > > + const char *s = devstr; > > + size_t nblayer; > > + size_t i = 0; > > + > > + /* Having both busstr and clsstr NULL is illegal, > > + * marking this iterator as invalid unless > > + * everything goes well. > > + */ > > + it->busstr = NULL; > > + it->clsstr = NULL; > > + /* Split each sub-lists. */ > > + nblayer = dev_layer_count(devstr); > > + if (nblayer > RTE_DIM(layers)) { > > + RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n", > > + nblayer); > > + rte_errno = EINVAL; > > + goto get_out; > > This one could just return -EINVAL; instead of calling get_out. > > + } > > + while (s != NULL) { > > + char *copy; > > + char *end; > > + > > + if (strncmp(layers[i].key, s, > > + strlen(layers[i].key))) > > + goto next_layer; > > + layers[i].str = s; > > + copy = strdup(s); > > + if (copy == NULL) { > > + RTE_LOG(ERR, EAL, "OOM\n”); > > Maybe spell it out in the log ‘Out of Memory’. Will do. > > + rte_errno = ENOMEM; > > + goto get_out; > > + } > > + end = strchr(copy, '/'); > > + end = end ? end : strchr(copy, '\0'); > > + end[0] = '\0'; > > + layers[i].kvlist = rte_kvargs_parse(copy, NULL); > > + free(copy); > > I am sorry this method of not adding blank lines is a bit silly and we need to rethink at least adding a few blank lines to help with grouping the code. This style will cause problems for new readers (old readers) to understand the code. This to me is a maintenance problem for the future and we need to fix this now. > > Blank lines after if statements (with possible more comments) can help along with adding blank lines to group code is really the minimum amount of lines required. I have never seen someone state you have to many blanks lines in the code except two or more blank lines in a row. This is akin to having a single paragraph in a novel or letter and it makes it very hard to read. It is not hard to add some blank lines to the code for readability. > I understand. What I dislike is having inconsistencies in the layout of the code. "Paragraphs", for lack of a better word, are high subjective and a matter of taste. Given that subjectivity is not helpful in review and taste is hard to debate, I prefer to have a single terse rule, that is to avoid any additional bytes beside the bare minimum to respect checkpatch. When I feel the need to have a new "conceptual group", then I am forced to add a comment to explain what is happening. By not allowing blank lines, I thus feel the pressure for space and explanation more acutely. Also, this makes a patch context as information-rich as possible. > I stopped reviewing the code as it became difficult to read IMO and just causing me a headache. > > This problem needs to be addressed by the TSC IMO. Sorry that it made it more difficult than necessary. It would be interesting if an addendum to the kernel coding style was made regarding blank lines. Regards, -- Gaëtan Rivet 6WIND