Re: [edk2-devel] [PATCH 19/35] NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces calls

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/30/19 10:16 PM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 09/26/19 14:42, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 9/17/19 9:49 PM, Laszlo Ersek wrote:
>>> Both the "ControllerHandle" parameter of CloseProtocol()
>>
>> Maybe worth adding "of type EFI_CLOSE_PROTOCOL"
>>
>>> and the "Handle"
>>> parameter of UninstallMultipleProtocolInterfaces()
>>
>> "of type EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES"
> 
> I don't think these changes would add much information.
> 
> The above names are -- strictly speaking -- field names in the
> EFI_BOOT_SERVICES structure. However, in UEFI code, they are so
> frequently used that people simply refer to them
> - either by just the member name,
> - or by the pointer type *in itself*.
> 
> So using EFI_CLOSE_PROTOCOL in itself would certainly be correct and
> directly understandable, same as CloseProtocol(). Using both at the same
> type is redundant.
> 
> You can observe this simplification in the spec as well, BTW. For
> example, in UEFI-2.8 "2.5.4 Device Drivers", we find
> 
>      OpenProtocol() and CloseProtocol() update the handle database
>      maintained by the system firmware to track which drivers are
>      consuming protocol interfaces.

Fine.

> Another comment below:
> 
>>
>> have type EFI_HANDLE,
>>> not (EFI_HANDLE*).
>>>
>>> This patch fixes actual bugs. The issues have been dormant likely because
>>> they are on error paths. (Or, in case of TlsAuthConfigDxe, because the
>>> driver is unloaded likely very infrequently.)
>>>
>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>      build-tested only
>>>
>>>   NetworkPkg/DnsDxe/DnsDriver.c                  | 4 ++--
>>>   NetworkPkg/IScsiDxe/IScsiConfig.c              | 2 +-
>>>   NetworkPkg/Ip4Dxe/Ip4Driver.c                  | 2 +-
>>>   NetworkPkg/Ip6Dxe/Ip6Driver.c                  | 2 +-
>>>   NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c            | 2 +-
>>>   NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +-
>>>   6 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/NetworkPkg/DnsDxe/DnsDriver.c b/NetworkPkg/DnsDxe/DnsDriver.c
>>> index 94d072159a4d..ad007da8b7d6 100644
>>> --- a/NetworkPkg/DnsDxe/DnsDriver.c
>>> +++ b/NetworkPkg/DnsDxe/DnsDriver.c
>>> @@ -1145,7 +1145,7 @@ Dns4ServiceBindingCreateChild (
>>>              DnsSb->ConnectUdp->UdpHandle,
>>>              &gEfiUdp4ProtocolGuid,
>>>              gDns4DriverBinding.DriverBindingHandle,
>>> -           ChildHandle
>>> +           *ChildHandle
>>
>> EFI_CLOSE_PROTOCOL, OK.
>>
>>>              );
>>>   
>>>        gBS->UninstallMultipleProtocolInterfaces (
>>> @@ -1388,7 +1388,7 @@ Dns6ServiceBindingCreateChild (
>>>              DnsSb->ConnectUdp->UdpHandle,
>>>              &gEfiUdp6ProtocolGuid,
>>>              gDns6DriverBinding.DriverBindingHandle,
>>> -           ChildHandle
>>> +           *ChildHandle
>>
>> EFI_CLOSE_PROTOCOL, OK.
>>
>>>              );
>>>   
>>>        gBS->UninstallMultipleProtocolInterfaces (
>>> diff --git a/NetworkPkg/IScsiDxe/IScsiConfig.c b/NetworkPkg/IScsiDxe/IScsiConfig.c
>>> index b876da7f5ccd..d773849fd3b0 100644
>>> --- a/NetworkPkg/IScsiDxe/IScsiConfig.c
>>> +++ b/NetworkPkg/IScsiDxe/IScsiConfig.c
>>> @@ -3852,7 +3852,7 @@ IScsiConfigFormInit (
>>>                                        );
>>>     if (CallbackInfo->RegisteredHandle == NULL) {
>>>       gBS->UninstallMultipleProtocolInterfaces (
>>> -           &CallbackInfo->DriverHandle,
>>> +           CallbackInfo->DriverHandle,
>>
>> EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.
>>
>>>              &gEfiDevicePathProtocolGuid,
>>>              &mIScsiHiiVendorDevicePath,
>>>              &gEfiHiiConfigAccessProtocolGuid,
>>> diff --git a/NetworkPkg/Ip4Dxe/Ip4Driver.c b/NetworkPkg/Ip4Dxe/Ip4Driver.c
>>> index ebd4dec1dfe4..62be8b681a18 100644
>>> --- a/NetworkPkg/Ip4Dxe/Ip4Driver.c
>>> +++ b/NetworkPkg/Ip4Dxe/Ip4Driver.c
>>> @@ -891,7 +891,7 @@ Ip4ServiceBindingCreateChild (
>>>                     );
>>>     if (EFI_ERROR (Status)) {
>>>       gBS->UninstallMultipleProtocolInterfaces (
>>> -           ChildHandle,
>>> +           *ChildHandle,
>>
>> EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.
>>
>>>              &gEfiIp4ProtocolGuid,
>>>              &IpInstance->Ip4Proto,
>>>              NULL
>>> diff --git a/NetworkPkg/Ip6Dxe/Ip6Driver.c b/NetworkPkg/Ip6Dxe/Ip6Driver.c
>>> index 7dc9e45af7b6..63d8428dbced 100644
>>> --- a/NetworkPkg/Ip6Dxe/Ip6Driver.c
>>> +++ b/NetworkPkg/Ip6Dxe/Ip6Driver.c
>>> @@ -888,7 +888,7 @@ Ip6ServiceBindingCreateChild (
>>>                     );
>>>     if (EFI_ERROR (Status)) {
>>>       gBS->UninstallMultipleProtocolInterfaces (
>>> -           ChildHandle,
>>> +           *ChildHandle,
>>
>> EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.
>>
>>>              &gEfiIp6ProtocolGuid,
>>>              &IpInstance->Ip6Proto,
>>>              NULL
>>> diff --git a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
>>> index ae9e65544a86..06c4e202d3ef 100644
>>> --- a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
>>> +++ b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
>>> @@ -592,7 +592,7 @@ Mtftp4ServiceBindingCreateChild (
>>>              MtftpSb->ConnectUdp->UdpHandle,
>>>              &gEfiUdp4ProtocolGuid,
>>>              gMtftp4DriverBinding.DriverBindingHandle,
>>> -           ChildHandle
>>> +           *ChildHandle
>>
>> EFI_CLOSE_PROTOCOL, OK.
>>
>>>              );
>>>       goto ON_ERROR;
>>>     }
>>> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
>>> index 18ee763002b4..c0870ab9979c 100644
>>> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
>>> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
>>> @@ -39,7 +39,7 @@ TlsAuthConfigDxeUnload (
>>>     ASSERT (PrivateData->Signature == TLS_AUTH_CONFIG_PRIVATE_DATA_SIGNATURE);
>>>   
>>>     gBS->UninstallMultipleProtocolInterfaces (
>>> -         &ImageHandle,
>>> +         ImageHandle,
>>
>> EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.
>>
>>>            &gEfiCallerIdGuid,
>>>            PrivateData,
>>>            NULL
>>>
>>
>> I'd have split this patch in 2 for easier review (one fixing
>> EFI_CLOSE_PROTOCOL, another fixing
>> EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES).
>>
>> As it or split:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Splitting these patches the right way was a difficult question for me. I
> didn't want to give each fix its own patch (because, presumably, nobody
> likes a ~100-patch series, with one-liner fixes, except me, I guess... I
> do like that, after all that's how I fixed the issues, one by one). And
> once I opted for a coarser granularity, it was hard to tell how much
> coarser it should get. As you can see, it varies over the series --
> sometimes it's Pkg-level, sometimes it's module-level...
> 
> Unless there's a critical issue, I'd like to stick with the present
> version. (Managing the feedback tags manually hasn't been trivial!) So
> I'll take your R-b for the patch as-is, if that's OK with you.

Sure, no problem!

I guess I reviewed ~32/35 patches of this series, so I could feel your pain.

Regards,

Phil.