[edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in UninstallMultipleProtocol follow Spec

[edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in UninstallMultipleProtocol follow Spec

[Heinrich Schuchardt]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1869

The UEFI spec requires that if any error occurs in
UninstallMultipleProtocolInterfaces(), EFI_INVALID_PARAMETER is returned
and not the return code of UninstallProtocolInterface().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	Adjust the subject line.
	Adjust the function comments to clarify the behavior.
	This replaces https://edk2.groups.io/g/devel/message/46974
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index b2721b3ab2..719ba98261 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -802,20 +802,28 @@ Done:
 
 
 
-
 /**
   Uninstalls a list of protocol interface in the boot services environment.
-  This function calls UnisatllProtocolInterface() in a loop. This is
+  This function calls UninstallProtocolInterface() in a loop. This is
   basically a lib function to save space.
 
-  @param  Handle                 The handle to uninstall the protocol
+  If any errors are generated while the protocol interfaces are being
+  uninstalled, then the protocol interfaces uninstalled prior to the error will
+  be reinstalled in reverse order of uninstalling and EFI_INVALID_PARAMETER is
+  returned.
+
+  @param  Handle                 The handle to uninstall the protocol interfaces
+                                 from.
   @param  ...                    EFI_GUID followed by protocol instance. A NULL
-                                 terminates the  list. The pairs are the
+                                 terminates the list. The pairs are the
                                  arguments to UninstallProtocolInterface(). All
                                  the protocols are added to Handle.
 
-  @return Status code
-
+  @return EFI_SUCCESS            if all protocol interfaces where uninstalled.
+  @return EFI_INVALID_PARAMETER  if any protocol interface could not be
+                                 uninstalled and an attempt was made to
+                                 reinstall previously uninstalled protocol
+                                 interfaces.
 **/
 EFI_STATUS
 EFIAPI
@@ -864,6 +872,7 @@ CoreUninstallMultipleProtocolInterfaces (
       CoreInstallProtocolInterface (&Handle, Protocol, EFI_NATIVE_INTERFACE, Interface);
     }
     VA_END (Args);
+    Status = EFI_INVALID_PARAMETER;
   }
 
   return Status;
-- 
2.20.1

Re: [edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in UninstallMultipleProtocol follow Spec

[Dandan Bi]

Hi ,

Thanks for the update. 
I don't know how you send this patch, the format and subject seems a little different from V1 patch. Anyway it doesn't block my review of this patch.

I have two minor comments for the function comments:
1. Per EDKII C Coding Spec, @retval for each unique return value, @return for a function's return values when those values aren't easily described by @retval commands.
So here I think we should use @retval instead of @return.
2. Per my understanding, the protocol interfaces uninstalled prior to the error seems not to be reinstalled in reverse order of uninstalling, the reinstallation seems in the same order of previous uninstalling (start from the first pair after the handle to next...). Please help double check this issue and update the comments if needed .

Since these are the comments update and with these addressed,  Reviewed-by: Dandan Bi <dandan.bi@intel.com>

Thanks,
Dandan
> -----Original Message-----
> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
> Sent: Tuesday, September 10, 2019 4:12 PM
> To: EDK II Development <devel@edk2.groups.io>; Bi, Dandan
> <dandan.bi@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Jin, Eric
> <eric.jin@intel.com>; Supreeth Venkatesh <supreeth.venkatesh@arm.com>;
> Stephano Cetola <stephano.cetola@linux.intel.com>; Heinrich Schuchardt
> <xypron.glpk@gmx.de>
> Subject: [edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in
> UninstallMultipleProtocol follow Spec
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1869
> 
> The UEFI spec requires that if any error occurs in
> UninstallMultipleProtocolInterfaces(), EFI_INVALID_PARAMETER is returned
> and not the return code of UninstallProtocolInterface().
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	Adjust the subject line.
> 	Adjust the function comments to clarify the behavior.
> 	This replaces https://edk2.groups.io/g/devel/message/46974
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> index b2721b3ab2..719ba98261 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> @@ -802,20 +802,28 @@ Done:
>    - /**   Uninstalls a list of protocol interface in the boot services
> environment.-  This function calls UnisatllProtocolInterface() in a loop. This
> is+  This function calls UninstallProtocolInterface() in a loop. This is   basically a
> lib function to save space. -  @param  Handle                 The handle to uninstall
> the protocol+  If any errors are generated while the protocol interfaces are
> being+  uninstalled, then the protocol interfaces uninstalled prior to the error
> will+  be reinstalled in reverse order of uninstalling and
> EFI_INVALID_PARAMETER is+  returned.++  @param  Handle                 The
> handle to uninstall the protocol interfaces+                                 from.   @param  ...
> EFI_GUID followed by protocol instance. A NULL-                                 terminates
> the  list. The pairs are the+                                 terminates the list. The pairs are
> the                                  arguments to UninstallProtocolInterface(). All
> the protocols are added to Handle. -  @return Status code-+  @return
> EFI_SUCCESS            if all protocol interfaces where uninstalled.+  @return
> EFI_INVALID_PARAMETER  if any protocol interface could not be+
> uninstalled and an attempt was made to+                                 reinstall previously
> uninstalled protocol+                                 interfaces. **/ EFI_STATUS EFIAPI@@ -
> 864,6 +872,7 @@ CoreUninstallMultipleProtocolInterfaces (
>        CoreInstallProtocolInterface (&Handle, Protocol, EFI_NATIVE_INTERFACE,
> Interface);     }     VA_END (Args);+    Status = EFI_INVALID_PARAMETER;   }
> return Status;--
> 2.20.1

Re: [edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in UninstallMultipleProtocol follow Spec

[Heinrich Schuchardt]

On 9/12/19 3:59 AM, Bi, Dandan wrote:
> Hi ,
>
> Thanks for the update.
> I don't know how you send this patch, the format and subject seems a little different from V1 patch. Anyway it doesn't block my review of this patch.
>
> I have two minor comments for the function comments:
> 1. Per EDKII C Coding Spec, @retval for each unique return value, @return for a function's return values when those values aren't easily described by @retval commands.

Thanks for pointing me to the coding spec (available at
https://github.com/tianocore-docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf).

I will correct that. Looking at the rest of the file you will other
examples of incorrect use of @return: CoreUnregisterProtocolNotifyEvent.

> So here I think we should use @retval instead of @return.
> 2. Per my understanding, the protocol interfaces uninstalled prior to the error seems not to be reinstalled in reverse order of uninstalling, the reinstallation seems in the same order of previous uninstalling (start from the first pair after the handle to next...). Please help double check this issue and update the comments if needed .

You are right. VA_ARG() moves form first to last.

Thanks for reviewing. Best regards

Heinrich

>
> Since these are the comments update and with these addressed,  Reviewed-by: Dandan Bi <dandan.bi@intel.com>
>
> Thanks,
> Dandan
>> -----Original Message-----
>> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
>> Sent: Tuesday, September 10, 2019 4:12 PM
>> To: EDK II Development <devel@edk2.groups.io>; Bi, Dandan
>> <dandan.bi@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
>> Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Yao,
>> Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Jin, Eric
>> <eric.jin@intel.com>; Supreeth Venkatesh <supreeth.venkatesh@arm.com>;
>> Stephano Cetola <stephano.cetola@linux.intel.com>; Heinrich Schuchardt
>> <xypron.glpk@gmx.de>
>> Subject: [edk2-core] [PATCH v2 1/1] MdeModulePkg: Make retval in
>> UninstallMultipleProtocol follow Spec
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1869
>>
>> The UEFI spec requires that if any error occurs in
>> UninstallMultipleProtocolInterfaces(), EFI_INVALID_PARAMETER is returned
>> and not the return code of UninstallProtocolInterface().
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>> 	Adjust the subject line.
>> 	Adjust the function comments to clarify the behavior.
>> 	This replaces https://edk2.groups.io/g/devel/message/46974
>> ---
>>   MdeModulePkg/Core/Dxe/Hand/Handle.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
>> b/MdeModulePkg/Core/Dxe/Hand/Handle.c
>> index b2721b3ab2..719ba98261 100644
>> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
>> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
>> @@ -802,20 +802,28 @@ Done:
>>     - /**   Uninstalls a list of protocol interface in the boot services
>> environment.-  This function calls UnisatllProtocolInterface() in a loop. This
>> is+  This function calls UninstallProtocolInterface() in a loop. This is   basically a
>> lib function to save space. -  @param  Handle                 The handle to uninstall
>> the protocol+  If any errors are generated while the protocol interfaces are
>> being+  uninstalled, then the protocol interfaces uninstalled prior to the error
>> will+  be reinstalled in reverse order of uninstalling and
>> EFI_INVALID_PARAMETER is+  returned.++  @param  Handle                 The
>> handle to uninstall the protocol interfaces+                                 from.   @param  ...
>> EFI_GUID followed by protocol instance. A NULL-                                 terminates
>> the  list. The pairs are the+                                 terminates the list. The pairs are
>> the                                  arguments to UninstallProtocolInterface(). All
>> the protocols are added to Handle. -  @return Status code-+  @return
>> EFI_SUCCESS            if all protocol interfaces where uninstalled.+  @return
>> EFI_INVALID_PARAMETER  if any protocol interface could not be+
>> uninstalled and an attempt was made to+                                 reinstall previously
>> uninstalled protocol+                                 interfaces. **/ EFI_STATUS EFIAPI@@ -
>> 864,6 +872,7 @@ CoreUninstallMultipleProtocolInterfaces (
>>         CoreInstallProtocolInterface (&Handle, Protocol, EFI_NATIVE_INTERFACE,
>> Interface);     }     VA_END (Args);+    Status = EFI_INVALID_PARAMETER;   }
>> return Status;--
>> 2.20.1
>
>