From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=RzCtN/II; spf=pass (domain: linaro.org, ip: 209.85.128.68, mailfrom: leif.lindholm@linaro.org) Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by groups.io with SMTP; Wed, 10 Jul 2019 13:57:15 -0700 Received: by mail-wm1-f68.google.com with SMTP id 207so3628657wma.1 for ; Wed, 10 Jul 2019 13:57:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=/XjdawCqwZjvfU+QIjDN/Oyz7N3Thn3nxPlpioYSLro=; b=RzCtN/IID7hNJf9etBNjLZYJ8znVO1JokT1T6wxb61O1u2C8RNBgenZO+LT8wMo/fQ U29JYgPv6W9EDwXT1oxRtbzM34/YsKdSwWYrZ7P7Bzu1gcMza7L5Z+Tk1WsC4i8YnbyV 7CR2DQ2Y22jHi/wvOoZrf99c5N30jUCncGhr4Vy9hqjEfr3pqXiuPmXfOS8MG7Zi+bsS bqwKcc3lCFal9W7zksk0+7Mu3dcrJp+jvmpDkgEyTz+MSTGMN+GB7536as9CIhG0rFwZ 0OY4I1ejYCVsxx0jqqLyc/QmWF3aBNaak3VExxeg2NY94opCJA9ss8IO3NZgoS8dEzQW LCAA== 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:in-reply-to:user-agent; bh=/XjdawCqwZjvfU+QIjDN/Oyz7N3Thn3nxPlpioYSLro=; b=BK+B80Dm9GQLRkaSMfBOD5Blh3MOm9l6yrXswtVz+oc9NECJrQoFz8/+Iho4HAtcGh KPRxU4xMgC4J/KxJhuCgaYYB/Bd3Z+k7aiR0dLzXYsvb7ErXv7cKhkrA79Adw04eCIaB YZ5lzvaR1i2XL/iamw9wQZhrfxvSI7AQx/h2fK8+d5X8PhX/RA67ka0ZZJ73Us0VnmXG ZNHF95Pu9Z43w+0KztDW1z7FZbB6RG14laEWqrWGjQnzEUYx1IgXN53HR/oFEXziWZP7 l5Pabd1DC9pVIGAUfhfQjuPuj2yciNsl/Ni4c9FIY/0galXQr2TKYdxPSXx/10pFIKBj jMHg== X-Gm-Message-State: APjAAAUeq81mgB7LM6gShCcGP0raFD/F4+d5MgJqGnK+j8phKI1Hg+DW IUBL7f/ghY+2Q3qhde+pyRmpmQVgqkI= X-Google-Smtp-Source: APXvYqyQMxT0hNEQyF1fMkG6gMhQGosffNazWa/nlPXe3KXDjFTX2WKzNCRDpEnrpjykjOukIJFeMA== X-Received: by 2002:a1c:d185:: with SMTP id i127mr7026146wmg.63.1562792233408; Wed, 10 Jul 2019 13:57:13 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id p4sm3641414wrs.35.2019.07.10.13.57.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 10 Jul 2019 13:57:12 -0700 (PDT) Date: Wed, 10 Jul 2019 21:57:11 +0100 From: "Leif Lindholm" To: devel@edk2.groups.io, liming.gao@intel.com Cc: "Gao, Zhichao" , "Feng, Bob C" , Andrew Fish , Laszlo Ersek , "Kinney, Michael D" Subject: Re: [edk2-devel] [Patch v3] BaseTools: Fix GCC compiler failure in new added tools. Message-ID: <20190710205711.wqqzbvscbt4war6h@bivouac.eciton.net> References: <1562666013-13188-1-git-send-email-liming.gao@intel.com> <20190709182559.to6n5pifdkegc7yy@bivouac.eciton.net> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A53C2@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A53C2@SHSMSX104.ccr.corp.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jul 10, 2019 at 01:42:44PM +0000, Liming Gao wrote: > > > - strncat (LocalStr, "[attributes] \n", sizeof("[attributes] \n")); > > > + strncat (LocalStr, "[attributes] \n", STR_LEN_MAX_4K - strlen (LocalStr) - 1); > > > > This is a very inefficient, and difficult to read, way of doing this. > > It also performs absolutely no error checking (making any future > > debugging tedious at best). > > This change is to fix gcc warning -Werror=sizeof-pointer-memaccess. > strncat is the safe version API. So, it is used here. Yes, but this code is continuously appending to a long string, and then counting the length of that string. This could be done *very* much more efficiently by keeping track of the current position we're appending to and using strncpy. It would also improve readability. As for the safe version - my point about error checking was not to do with buffer overruns. It was to do with the fact that if the code ends up filling the buffer, that will go undetected until something goes wrong further along. > > I have to be honest - looking at this code, I think the right thing to > > do would be to revert the commit adding it and reworking the utility > > completely (making sure it gets serious review) before merging it. > > > > I am not surprised the compilers get upset. > > > > This made me have a closer look at the patches adding FCE and FMMT, > > and frankly, my opinion of them are similar. These aren't being > > upstreamed - this is textbook "chuck over the wall". > > > > Don't get me wrong - the code quality of FMMT is notably higher than > > the code quality of FCE, which is notably higher than the code quality > > of BfmLib. But even > > > > BfmLib was added with the comment that it is used by FCE. > > So why do FCE and FMMT both have separate copies of LibBfmGuidToStr? > > Not just functions doing the same thing, but with the same name. > > > > There are many other issues with these tools. > > FCE & FMMT both operate the binary FV image. Their implementation > may copy the same logic. I don't think this is a good way. > I agree to do better design for code sharing and code quality improvement. > > I suggest to revert them, add current FCE & FMMT into edk2-staging, > then refine them, and send the patch to add them back into edk2 > BaseTools. This sounds good to me - thanks! Best Regards, Leif