Re: [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant definition.

[Laszlo Ersek <lersek@redhat.com>]

On 08/17/18 03:36, Li, Songpeng wrote:
> The file "NetworkPkg/TlsDxe/TlsProtocol.c" which contains "NetworkPkg/TlsDxe/TlsImpl.h"
> is using the definition HTONS which defined in <Library/NetLib.h>
> TlsProtocol.c:
> Line 264      *((UINT16 *) Data) = HTONS (TlsGetVersion (Instance->TlsConn));
> NetLib.h:
> Line 230      #define HTONS(x)   NTOHS(x)
> 
> It cannot be removed. Please drop this patch.

Right, I imagined that the code could be using just some macros from the
lib class header, but no extern variables or functions. In such cases, I
prefer to keep the lib class dependency in the INF file as well -- for
example, the macro definition in the lib class header could change at
some point and expand to a call to a new function that is also declared
(as a new API) in the lib class header. In this case, even if all lib
classes were extended with the new API, the client code would no longer
link.

So, as a general rule, including a lib class header implies that the INF
file should specify the lib class dependency too.

I agree that we should drop this patch.

Thanks
Laszlo

> 
> 
> Thanks & Best Regards,
> Songpeng
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, August 17, 2018 3:27 AM
>> To: Li, Songpeng <songpeng.li@intel.com>; edk2-devel@lists.01.org
>> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: Re: [edk2] [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant
>> definition.
>>
>> On 08/16/18 03:37, Songpeng Li wrote:
>>> The definition NetLib has been removed.
>>
>> (1) I suggest calling this "lib class dependency", not "definition".
>>
>> (I could have made the same for a GUID removed in a previous patch, but
>> there "definition" seemed acceptable. "GUID dependency" would be an
>> improvement there as well.)
>>
>> The subject line could also say, "remove superfluous dependencies".
>>
>>>
>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1064
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Songpeng Li <songpeng.li@intel.com>
>>> ---
>>>  NetworkPkg/TlsDxe/TlsDxe.inf | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/NetworkPkg/TlsDxe/TlsDxe.inf b/NetworkPkg/TlsDxe/TlsDxe.inf
>>> index 907feb735b..aaea0fc2ff 100644
>>> --- a/NetworkPkg/TlsDxe/TlsDxe.inf
>>> +++ b/NetworkPkg/TlsDxe/TlsDxe.inf
>>> @@ -5,7 +5,7 @@
>>>  #  This module produces EFI TLS (Transport Layer Security) Protocol and EFI
>> TLS
>>>  #  Service Binding Protocol, to provide TLS services.
>>>  #
>>> -#  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>>> +#  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>>>  #
>>>  #  This program and the accompanying materials
>>>  #  are licensed and made available under the terms and conditions of the
>> BSD License
>>> @@ -52,7 +52,6 @@
>>>    BaseLib
>>>    UefiLib
>>>    DebugLib
>>> -  NetLib
>>>    BaseCryptLib
>>>    TlsLib
>>>
>>>
>>
>> (2) This patch is incomplete; the file "NetworkPkg/TlsDxe/TlsImpl.h"
>> contains:
>>
>> #include <Library/NetLib.h>
>>
>> That header file is huge, so I won't try to audit, on a source code
>> level, whether TlsDxe makes any NetLib API calls.
>>
>> (Note that even if it does, then the current patch need not necessarily
>> cause a linking failure. Because, a NetLib dependency could be inherited
>> through one of the library instances with which a given platform
>> resolves the *other* lib class dependencies for TlsDxe.)
>>
>> So, please remove the #include too, and if TlsDxe still compiles, please
>> resubmit the patch. (Otherwise, drop the patch, of course.)
>>
>> Thanks!
>> Laszlo