From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6904D21E87982 for ; Thu, 14 Sep 2017 01:23:16 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C14B281DF3; Thu, 14 Sep 2017 08:26:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C14B281DF3 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-83.rdu2.redhat.com [10.10.120.83]) by smtp.corp.redhat.com (Postfix) with ESMTP id 099C91715E; Thu, 14 Sep 2017 08:26:12 +0000 (UTC) To: "Wang, Jian J" Cc: "edk2-devel@lists.01.org" , "Justen, Jordan L" , "Dong, Eric" , "Kinney, Michael D" , "Wolman, Ayellet" , "Yao, Jiewen" , "Zeng, Star" References: <20170913092507.12504-1-jian.j.wang@intel.com> <20170913092507.12504-5-jian.j.wang@intel.com> <848bc70b-b3f7-62fb-ef2d-df89ff8805a6@redhat.com> From: Laszlo Ersek Message-ID: Date: Thu, 14 Sep 2017 10:26:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 14 Sep 2017 08:26:14 +0000 (UTC) Subject: Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Sep 2017 08:23:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/14/17 03:17, Wang, Jian J wrote: > Thanks for the comments and good advices. Sorry the format issues. > This is my first patch for this project. Too many details for me to get > familiar with. > > (1) Sure. > (2) I'll change that. > (3) I'll use the tool to ensure the patch format. > (4) I'll remove the ',' in name > (5) I'll add more description about it. > (6) You're right. I should use SetMemorySpaceAttributes() of DXE service > instead. The only reason I didn't do it is that I found > GetMemorySpaceDescriptor() doesn't return the same information > which SetMemorySpaceAttributes() just changed. So I feel using CPU > arch protocol is a bit safer. Anyway, I'll change it. > (7) I did put those macros in the install function before. To reduce the > number of changed files, I made current changes. You're right it's > not worthy. > (8) Using macro can help the readability, which is more important to me. > I know function can do the same. But it looks a bit heavy in this situation. > I have to admit replacing the macros with a library is a very good idea, > which brings the same readability. I didn't think of that before. Although > Library is still a little bit heavy to me but it's in a different way, I think it > worth a trying. > (9) Putting a space before open parenthesis is forced style? If so, I'll add it. Yes, it is in the CCS: https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/52_spacing.html#52-spacing > 5.2.2.6 Always put space before an open parenthesis > > The only exception is macro definitions. > > if (... > while (... > EfiLibAllocateCopyPool (... Thanks, Laszlo > (10) You're right. Using library can reduce the disturbs to affected drivers > by this feature to the minimum.