1. Patchew
  2. EDK2
  3. [edk2] [PATCH 0/5] NetworkPkg: Remove the redundant code and definition.
  4. View patch

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

Songpeng Li posted 5 patches 6 years ago
[edk2] [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant definition.
Posted by Songpeng Li 6 years ago 
The definition NetLib has been removed.

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.18.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant definition.
Posted by Laszlo Ersek 6 years ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant definition.
Posted by Li, Songpeng 6 years ago 
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.


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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/5] NetworkPkg: TlsDxe: Remove the redundant definition.
Posted by Laszlo Ersek 6 years ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.