From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id B100B8D8E for ; Wed, 18 Apr 2018 15:28:50 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Apr 2018 06:28:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,465,1517904000"; d="scan'208";a="48860493" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.51]) by orsmga001.jf.intel.com with SMTP; 18 Apr 2018 06:28:45 -0700 Received: by (sSMTP sendmail emulation); Wed, 18 Apr 2018 14:28:43 +0100 Date: Wed, 18 Apr 2018 14:28:43 +0100 From: Bruce Richardson To: "Kuusisaari, Juhamatti" Cc: Thomas Monjalon , Scott Branden , Stephen Hemminger , "dev@dpdk.org" Message-ID: <20180418132843.GA124804@bricha3-MOBL.ger.corp.intel.com> References: <20180417214919.8246-1-stephen@networkplumber.org> <2994859.WyYqfpDCHC@xps> <4245883.kl7A9e8lPX@xps> <20180418085605.GA111744@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [RFC] checkpatch: don't complain about SPDX tag format 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: Wed, 18 Apr 2018 13:28:51 -0000 On Wed, Apr 18, 2018 at 10:49:07AM +0000, Kuusisaari, Juhamatti wrote: > > Hello, > > > On Wed, Apr 18, 2018 at 12:19:07AM +0200, Thomas Monjalon wrote: > > > 18/04/2018 00:11, Scott Branden: > > > > On 18-04-17 03:06 PM, Thomas Monjalon wrote: > > > > > 17/04/2018 23:49, Stephen Hemminger: > > > > >> IMHO would have been better to use the kernel SPDX style and keep > > > > >> the check but that appears to be a minority opinion. > > > > > > > > > > I think it is better to work on checkpatch itself. > > > > > When defining our SPDX style, Linux one was not definitive. > > > > > Do you think we can ask the Linux community to support our SPDX style? > > > > > > > > > I think it better to simply follow the Linux community defacto style > > > > rather than go your own way. > > > > > > But our way is better! :) > > > And it has been decided in the Technical Board. > > > > > > > As a general issue, I think we could do with having our own checkpatch-like > > script for performing addition DPDK-specific code-checks *after* Linux > > checkpatch ones. That is, reuse Linux check patch checks as much as possible, > > but have other checks too. > > > > For example, check for use of strcpy or strncpy (or snprintf with "%s") and > > suggest replacing with strlcpy. If we did have our own extension script, we > > could put our own SPDX format check there too. > > > > Thoughts, or any volunteers to look into this? > > In addition, the checkpatches.sh could be improved so that it actually checks that a proper file is found behind the selected env variable. I am planning to add this check (as it bite me just yesterday). > > Speaking of strlcpy, I do think that it has a caveat* that everybody should be aware of: depending on implementation, it may read unintended memory regions when the source is not properly null terminated (like in Unix domain sockets, or just by other mistake). It may be a bad idea just blindly replace everything with strlcpy, without making sure that copied buffers are really null-terminated in the first place or making sure the strlcpy version is really a one that does not have this problem. As it depends on dynamic libraries, making sure may be difficult. > > Some may argue that this is unlikely and thus irrelevant. Why do I know about it then? :) Needless to say, strncpy or snprintf do not have _this_ problem, although they have their own issues. Internally without dynamic libs DPDK rte_strlcpy uses snprintf which should be safe, though. > > > /Bruce > > -- > Juhamatti > > * A caveat on some implementations: > ... > /* Not enough room in dst, add NUL and traverse rest of src */ > if (n == 0) { > if (siz != 0) > *d = '\0'; /* NUL-terminate dst */ > while (*s++) <- what happens when s is not null-terminated? > ; > } > ... > Another one: > ... > return n + strlen (src); <- what happens when src is not null-terminated? > ... Thanks for pointing that out. It's good to be aware of these caveats. I suspect in most cases the replacement is safe, but we should not blindly replace one thing with another without checking for possible unintended side effects. /Bruce