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: philmd@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 01 Oct 2019 00:18:57 -0700 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A0FD481F07 for ; Tue, 1 Oct 2019 07:18:56 +0000 (UTC) Received: by mail-wm1-f69.google.com with SMTP id k184so995877wmk.1 for ; Tue, 01 Oct 2019 00:18:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=9aVqFE8e13EXMEAC1I3rNLoPtuqckHMCYATNtenszSc=; b=P2Kv/M4Ice8EX7CT0R81u15b5ERWCnXT3EBrlyaUuMqZ4P+UqcG/tUjeDRp3yPrEnq jTYp5BqpRzE8hz3zmjw17k+VQNm7+ZyNErwllnKMK9vG/VFjkeOOC2QAbrxALWuJACFs ADoLZ96ol0o8N5xGkBq9mUnXVZaA8azoy+vp8nUuPMkSMYT7jm4MQcZ4oo7uppU1ttQq ShYyJEiFzS+hlzhaJsA2XFrpx+JON+SieOvu27Ik2cGlugrKPHLtdtW+djWqjsHjXpvY PkZ14bP9cb7R/XIVa3A7jv6E+KCYinn3w2wxjChCJAYPkXniu/PJDJHRZY6BYnisRMCT 8eww== X-Gm-Message-State: APjAAAXUCm8vbMUIhZ0bJ47JQ7ZDp+ATK+NdO6XFO8Ejmb2nDyJSaPuP 8gZfM/YHBhk67LIOMfkqxEUry29neyWKmhU1llWGUnwpPszAH4ph1vfQsJk3SCzRfhyUHwflbfi f4SCxIlgxhomNCQ== X-Received: by 2002:a5d:4646:: with SMTP id j6mr15654523wrs.173.1569914335292; Tue, 01 Oct 2019 00:18:55 -0700 (PDT) X-Google-Smtp-Source: APXvYqxpPw7+xwPZcCUzn6hPvL0D/aJbk7l2+LWgF6ffuM7p4lAbIcM415pN6a9kLcIbVXEjIdeGig== X-Received: by 2002:a5d:4646:: with SMTP id j6mr15654502wrs.173.1569914335066; Tue, 01 Oct 2019 00:18:55 -0700 (PDT) Received: from [192.168.1.35] (240.red-88-21-68.staticip.rima-tde.net. [88.21.68.240]) by smtp.gmail.com with ESMTPSA id 207sm3031225wme.17.2019.10.01.00.18.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Oct 2019 00:18:54 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 19/35] NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces calls To: Laszlo Ersek , devel@edk2.groups.io Cc: Jiaxin Wu , Siyuan Fu References: <20190917194935.24322-1-lersek@redhat.com> <20190917194935.24322-20-lersek@redhat.com> <4ba0e969-24a4-a940-8dc8-f1e754995267@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <04bf0fdd-fed3-e533-ed7c-b07b33a36e20@redhat.com> Date: Tue, 1 Oct 2019 09:18:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 MIME-Version: 1.0 In-Reply-To: <4ba0e969-24a4-a940-8dc8-f1e754995267@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 9/30/19 10:16 PM, Laszlo Ersek wrote: > Hi Phil, >=20 > On 09/26/19 14:42, Philippe Mathieu-Daud=C3=A9 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" >=20 > I don't think these changes would add much information. >=20 > 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*. >=20 > So using EFI_CLOSE_PROTOCOL in itself would certainly be correct and > directly understandable, same as CloseProtocol(). Using both at the sam= e > type is redundant. >=20 > You can observe this simplification in the spec as well, BTW. For > example, in UEFI-2.8 "2.5.4 Device Drivers", we find >=20 > OpenProtocol() and CloseProtocol() update the handle database > maintained by the system firmware to track which drivers are > consuming protocol interfaces. Fine. > Another comment below: >=20 >> >> have type EFI_HANDLE, >>> not (EFI_HANDLE*). >>> >>> This patch fixes actual bugs. The issues have been dormant likely bec= ause >>> they are on error paths. (Or, in case of TlsAuthConfigDxe, because th= e >>> driver is unloaded likely very infrequently.) >>> >>> Cc: Jiaxin Wu >>> Cc: Siyuan Fu >>> Signed-off-by: Laszlo Ersek >>> --- >>> >>> 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/DnsDri= ver.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. >> >>> ); >>> =20 >>> gBS->UninstallMultipleProtocolInterfaces ( >>> @@ -1388,7 +1388,7 @@ Dns6ServiceBindingCreateChild ( >>> DnsSb->ConnectUdp->UdpHandle, >>> &gEfiUdp6ProtocolGuid, >>> gDns6DriverBinding.DriverBindingHandle, >>> - ChildHandle >>> + *ChildHandle >> >> EFI_CLOSE_PROTOCOL, OK. >> >>> ); >>> =20 >>> 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 =3D=3D 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/Ip4Dri= ver.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/Ip6Dri= ver.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/Mtftp4D= xe/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/Network= Pkg/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 =3D=3D TLS_AUTH_CONFIG_PRIVATE_DAT= A_SIGNATURE); >>> =20 >>> 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=C3=A9 >=20 > 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... >=20 > 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 pa= in. Regards, Phil.