Re: [edk2-devel] [PATCH 12/16] OvmfPkg/EnrollDefaultKeys: describe functions with leading comment blocks

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

On 4/27/19 2:53 AM, Laszlo Ersek wrote:
> The GetExact(), GetSettings(), PrintSettings(), and ShellAppMain()
> functions lack leading comment blocks. Supply those.
> 
> While at it, make sure that every such comment block is preceded by two
> blank lines.
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1747
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 73 ++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> index e4f6a50e008b..07297c631f38 100644
> --- a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> @@ -13,16 +13,17 @@
>  #include <Library/DebugLib.h>                    // ASSERT()
>  #include <Library/MemoryAllocationLib.h>         // FreePool()
>  #include <Library/ShellCEntryLib.h>              // ShellAppMain()
>  #include <Library/UefiLib.h>                     // AsciiPrint()
>  #include <Library/UefiRuntimeServicesTableLib.h> // gRT
>  
>  #include "EnrollDefaultKeys.h"
>  
> +
>  /**
>    Enroll a set of certificates in a global variable, overwriting it.
>  
>    The variable will be rewritten with NV+BS+RT+AT attributes.
>  
>    @param[in] VariableName  The name of the variable to overwrite.
>  
>    @param[in] VendorGuid    The namespace (ie. vendor GUID) of the variable to
> @@ -188,16 +189,54 @@ Out:
>    if (EFI_ERROR (Status)) {
>      AsciiPrint ("error: %a(\"%s\", %g): %r\n", __FUNCTION__, VariableName,
>        VendorGuid, Status);
>    }
>    return Status;
>  }
>  
>  
> +/**
> +  Read a UEFI variable into a caller-allocated buffer, enforcing an exact size.
> +
> +  @param[in] VariableName  The name of the variable to read; passed to
> +                           gRT->GetVariable().
> +
> +  @param[in] VendorGuid    The vendor (namespace) GUID of the variable to read;
> +                           passed to gRT->GetVariable().
> +
> +  @param[out] Data         The caller-allocated buffer that is supposed to
> +                           receive the variable's contents. On error, the
> +                           contents of Data are indeterminate.
> +
> +  @param[in] DataSize      The size in bytes that the caller requires the UEFI
> +                           variable to have. The caller is responsible for
> +                           providing room for DataSize bytes in Data.
> +
> +  @param[in] AllowMissing  If FALSE, the variable is required to exist. If
> +                           TRUE, the variable is permitted to be missing.
> +
> +  @retval EFI_SUCCESS           The UEFI variable exists, has the required size
> +                                (DataSize), and has been read into Data.
> +
> +  @retval EFI_SUCCESS           The UEFI variable doesn't exist, and
> +                                AllowMissing is TRUE. DataSize bytes in Data
> +                                have been zeroed out.
> +
> +  @retval EFI_NOT_FOUND         The UEFI variable doesn't exist, and
> +                                AllowMissing is FALSE.
> +
> +  @retval EFI_BUFFER_TOO_SMALL  The UEFI variable exists, but its size is
> +                                greater than DataSize.
> +
> +  @retval EFI_PROTOCOL_ERROR    The UEFI variable exists, but its size is
> +                                smaller than DataSize.
> +
> +  @return                       Error codes propagated from gRT->GetVariable().
> +**/
>  STATIC
>  EFI_STATUS
>  GetExact (
>    IN CHAR16   *VariableName,
>    IN EFI_GUID *VendorGuid,
>    OUT VOID    *Data,
>    IN UINTN    DataSize,
>    IN BOOLEAN  AllowMissing
> @@ -223,16 +262,41 @@ GetExact (
>      AsciiPrint ("error: GetVariable(\"%s\", %g): expected size 0x%Lx, "
>        "got 0x%Lx\n", VariableName, VendorGuid, (UINT64)DataSize, (UINT64)Size);
>      return EFI_PROTOCOL_ERROR;
>    }
>  
>    return EFI_SUCCESS;
>  }
>  
> +
> +/**
> +  Populate a SETTINGS structure from the underlying UEFI variables.
> +
> +  The following UEFI variables are standard variables:
> +  - L"SetupMode"  (EFI_SETUP_MODE_NAME)
> +  - L"SecureBoot" (EFI_SECURE_BOOT_MODE_NAME)
> +  - L"VendorKeys" (EFI_VENDOR_KEYS_VARIABLE_NAME)
> +
> +  The following UEFI variables are edk2 extensions:
> +  - L"SecureBootEnable" (EFI_SECURE_BOOT_ENABLE_NAME)
> +  - L"CustomMode"       (EFI_CUSTOM_MODE_NAME)
> +
> +  The L"SecureBootEnable" UEFI variable is permitted to be missing, in which
> +  case the corresponding field in the SETTINGS object will be zeroed out. The
> +  rest of the covered UEFI variables are required to exist; otherwise, the
> +  function will fail.
> +
> +  @param[out] Settings  The SETTINGS object to fill.
> +
> +  @retval EFI_SUCCESS  Settings has been populated.
> +
> +  @return              Error codes propagated from the GetExact() function. The
> +                       contents of Settings are indeterminate.
> +**/
>  STATIC
>  EFI_STATUS
>  GetSettings (
>    OUT SETTINGS *Settings
>    )
>  {
>    EFI_STATUS Status;
>  
> @@ -261,28 +325,37 @@ GetSettings (
>      return Status;
>    }
>  
>    Status = GetExact (EFI_VENDOR_KEYS_VARIABLE_NAME, &gEfiGlobalVariableGuid,
>               &Settings->VendorKeys, sizeof Settings->VendorKeys, FALSE);
>    return Status;
>  }
>  
> +
> +/**
> +  Print the contents of a SETTINGS structure to the UEFI console.
> +
> +  @param[in] Settings  The SETTINGS object to print the contents of.
> +**/
>  STATIC
>  VOID
>  PrintSettings (
>    IN CONST SETTINGS *Settings
>    )
>  {
>    AsciiPrint ("info: SetupMode=%d SecureBoot=%d SecureBootEnable=%d "
>      "CustomMode=%d VendorKeys=%d\n", Settings->SetupMode, Settings->SecureBoot,
>      Settings->SecureBootEnable, Settings->CustomMode, Settings->VendorKeys);
>  }
>  
>  
> +/**
> +  Entry point function of this shell application.
> +**/
>  INTN
>  EFIAPI
>  ShellAppMain (
>    IN UINTN  Argc,
>    IN CHAR16 **Argv
>    )
>  {
>    EFI_STATUS Status;
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>