Re: [PATCH] MdeModulePkg/UefiBootManagerLib: fix AddLoadOptionVariable docs/prototype

["Zeng, Star" <star.zeng@intel.com>]

Reviewed-by: Star Zeng <star.zeng@intel.com>


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, April 20, 2018 10:56 PM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: fix AddLoadOptionVariable docs/prototype

Ping -- can I get a review for this please; it's a trivial patch.

Thanks
Laszlo

On 04/17/18 11:30, Laszlo Ersek wrote:
> Clean up the leading comment and the prototype of
> EfiBootManagerAddLoadOptionVariable():
> 
> - the function may modify Option on output, annotate the parameter with
>   OUT and update the documentation;
> 
> - "@retval EFI_STATUS" and "@retval Others" are not idiomatic
>   documentation, use @return instead;
> 
> - sync comment and prototype between lib instance and lib class header.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: add_load_opt_inout
> 
>  MdeModulePkg/Include/Library/UefiBootManagerLib.h  | 26 +++++++++++++++-------
>  .../Library/UefiBootManagerLib/BmLoadOption.c      | 12 ++++++----
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> index 97ac1f233ce9..1d862a4b2684 100644
> --- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> +++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> @@ -176,20 +176,30 @@ EfiBootManagerLoadOptionToVariable (
>    );
>  
>  /**
> -  This function will update the Boot####/Driver####/SysPrep#### and the 
> -  BootOrder/DriverOrder/SysPrepOrder to add a new load option.
> +  This function will register the new Boot####, Driver#### or SysPrep#### option.
> +  After the *#### is updated, the *Order will also be updated.
>  
> -  @param  Option        Pointer to load option to add.
> -  @param  Position      Position of the new load option to put in the BootOrder/DriverOrder/SysPrepOrder.
> +  @param  Option            Pointer to load option to add. If on input
> +                            Option->OptionNumber is LoadOptionNumberUnassigned,
> +                            then on output Option->OptionNumber is updated to
> +                            the number of the new Boot####,
> +                            Driver#### or SysPrep#### option.
> +  @param  Position          Position of the new load option to put in the ****Order variable.
> +
> +  @retval EFI_SUCCESS           The *#### have been successfully registered.
> +  @retval EFI_INVALID_PARAMETER The option number exceeds 0xFFFF.
> +  @retval EFI_ALREADY_STARTED   The option number of Option is being used already.
> +                                Note: this API only adds new load option, no replacement support.
> +  @retval EFI_OUT_OF_RESOURCES  There is no free option number that can be used when the
> +                                option number specified in the Option is LoadOptionNumberUnassigned.
> +  @return                       Status codes of gRT->SetVariable ().
>  
> -  @retval EFI_SUCCESS   The load option has been successfully added.
> -  @retval Others        Error status returned by RT->SetVariable.
>  **/
>  EFI_STATUS
>  EFIAPI
>  EfiBootManagerAddLoadOptionVariable (
> -  IN EFI_BOOT_MANAGER_LOAD_OPTION  *Option,
> -  IN UINTN                         Position
> +  IN OUT EFI_BOOT_MANAGER_LOAD_OPTION *Option,
> +  IN     UINTN                        Position
>    );
>  
>  /**
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> index 32918caf324c..f88f8e02451c 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> @@ -329,7 +329,11 @@ BmAddOptionNumberToOrderVariable (
>    This function will register the new Boot####, Driver#### or SysPrep#### option.
>    After the *#### is updated, the *Order will also be updated.
>  
> -  @param  Option            Pointer to load option to add.
> +  @param  Option            Pointer to load option to add. If on input
> +                            Option->OptionNumber is LoadOptionNumberUnassigned,
> +                            then on output Option->OptionNumber is updated to
> +                            the number of the new Boot####,
> +                            Driver#### or SysPrep#### option.
>    @param  Position          Position of the new load option to put in the ****Order variable.
>  
>    @retval EFI_SUCCESS           The *#### have been successfully registered.
> @@ -338,14 +342,14 @@ BmAddOptionNumberToOrderVariable (
>                                  Note: this API only adds new load option, no replacement support.
>    @retval EFI_OUT_OF_RESOURCES  There is no free option number that can be used when the
>                                  option number specified in the Option is LoadOptionNumberUnassigned.
> -  @retval EFI_STATUS            Return the status of gRT->SetVariable ().
> +  @return                       Status codes of gRT->SetVariable ().
>  
>  **/
>  EFI_STATUS
>  EFIAPI
>  EfiBootManagerAddLoadOptionVariable (
> -  IN EFI_BOOT_MANAGER_LOAD_OPTION *Option,
> -  IN UINTN                        Position
> +  IN OUT EFI_BOOT_MANAGER_LOAD_OPTION *Option,
> +  IN     UINTN                        Position
>    )
>  {
>    EFI_STATUS                      Status;
>