From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Wed, 15 May 2019 01:10:17 -0700
Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id A05CC3082129;
	Wed, 15 May 2019 08:10:16 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-121.rdu2.redhat.com [10.10.120.121])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 1744C60F9C;
	Wed, 15 May 2019 08:10:14 +0000 (UTC)
Subject: Re: [edk2-devel] [PATCH 1/4] OvmfPkg: Update DSC/FDF to use NetworkPkg's include fragment file.
To: "Zhang, Shenglei" <shenglei.zhang@intel.com>,
 "devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Justen, Jordan L" <jordan.l.justen@intel.com>,
 Ard Biesheuvel <ard.biesheuvel@linaro.org>,
 Anthony Perard <anthony.perard@citrix.com>,
 Julien Grall <julien.grall@linaro.org>
References: <20190514073938.21528-1-shenglei.zhang@intel.com>
 <20190514073938.21528-2-shenglei.zhang@intel.com>
 <bd2612e9-7a2e-69eb-9cdd-5d622637793d@redhat.com>
 <C0706E73DB8C124D9B9C38AA364E5D5E04C31452@shsmsx102.ccr.corp.intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <d10d1257-4ba9-4009-3b68-96fb6d4e70a1@redhat.com>
Date: Wed, 15 May 2019 10:10:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <C0706E73DB8C124D9B9C38AA364E5D5E04C31452@shsmsx102.ccr.corp.intel.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 15 May 2019 08:10:16 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 05/15/19 09:02, Zhang, Shenglei wrote:
> Hi Laszlo:
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Tuesday, May 14, 2019 9:49 PM
>> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Anthony Perard <anthony.perard@citrix.com>;
>> Julien Grall <julien.grall@linaro.org>
>> Subject: Re: [edk2-devel] [PATCH 1/4] OvmfPkg: Update DSC/FDF to use
>> NetworkPkg's include fragment file.

>> (4) Therefore, please *prepend* a patch to this series that eliminates
>> the [LibraryClasses.common.DXE_DRIVER] resolutions altogether, at first.
> 
> Thanks for your comments!
> I'll send a patch first to solve the duplicated library classes about network in
> [LibraryClasses.common.DXE_DRIVER] section.

Thanks for that, I'll review it soon.

>> (6) Please be consistent with the comments that you add near the
>> !include directives. In the current patch, you add comments for the libs
>> and the PCDs, but not for the defines or the components. Please stick
>> with one style -- either add zero comments, or add comments on all of
>> the !includes.
> 
> Actually, for components I add "Network Support" in the comments, which looks like
> aligning with nearby components. Do you thinks it is acceptable?

Sorry, I missed the pre-existent comment there, and the fact that you
were preserving the comment.

So yes, that is fine -- as long as all !include directives uniformly
have a comment (newly added, or inherited from preexistent code), it's OK.

Please note however that your network *defines* !include still lacks a
comment! Please fix that, for consistency with the rest of the !include
directives.

Thanks
Laszlo