Re: [PATCH] OvmfPkg/QemuKernelLoaderFsDxe: drop tentative const object definition

["Laszlo Ersek" <lersek@redhat.com>]

On 03/06/20 08:38, Ard Biesheuvel wrote:
> Bob reports that VS2017 chokes on a tentative definition of the const
> object 'mEfiFileProtocolTemplate', with the following error:
> 
>   OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe.c(130):
>       error C2220: warning treated as error - no 'object' file generated
>   OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe.c(130):
>       warning C4132: 'mEfiFileProtocolTemplate': const object should be initialized
> 
> Let's turn the only function that relies on this tentative definition
> into a forward declaration itself, and move its definition after the
> normal definition of the object. That allows us to drop the tentative

(1) s/normal/external/

> definition of the const object, and hopefully make VS2017 happy.
> 
> Cc: "Feng, Bob C" <bob.c.feng@intel.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 134 ++++++++++----------
>  1 file changed, 70 insertions(+), 64 deletions(-)

I agree that this is a bug in VS2017.

I've re-checked "6.9.2 External object definitions" in C99 now, and the
original code is correct. So is the commit message on the present patch.

> 
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index 869549f164f0..fbcdf019bf56 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -123,13 +123,6 @@ typedef struct {
>  #define STUB_FILE_FROM_FILE(FilePointer) \
>          CR (FilePointer, STUB_FILE, File, STUB_FILE_SIG)
>  
> -//
> -// Tentative definition of the file protocol template. The initializer
> -// (external definition) will be provided later.
> -//
> -STATIC CONST EFI_FILE_PROTOCOL mEfiFileProtocolTemplate;
> -
> -
>  //
>  // Protocol member functions for File.
>  //
> @@ -181,65 +174,10 @@ StubFileOpen (
>    IN CHAR16             *FileName,
>    IN UINT64             OpenMode,
>    IN UINT64             Attributes
> -  )
> -{
> -  CONST STUB_FILE *StubFile;
> -  UINTN           BlobType;
> -  STUB_FILE       *NewStubFile;
> -
> +  );
>    //
> -  // We're read-only.
> +  // Forward declaration.
>    //

(2) I suggest replacing this comment (which follows the function
declaration) with a small insertion to the normal leading comment (which
is already there):

/**
  Opens a new file relative to the source file's location.

  (Forward declaration.) <--- this

  @param[in]  This ...
**/


With (1) clarified, and (2) optionally fixed (no need to repost):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo




> -  switch (OpenMode) {
> -    case EFI_FILE_MODE_READ:
> -      break;
> -
> -    case EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE:
> -    case EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE:
> -      return EFI_WRITE_PROTECTED;
> -
> -    default:
> -      return EFI_INVALID_PARAMETER;
> -  }
> -
> -  //
> -  // Only the root directory supports opening files in it.
> -  //
> -  StubFile = STUB_FILE_FROM_FILE (This);
> -  if (StubFile->BlobType != KernelBlobTypeMax) {
> -    return EFI_UNSUPPORTED;
> -  }
> -
> -  //
> -  // Locate the file.
> -  //
> -  for (BlobType = 0; BlobType < KernelBlobTypeMax; ++BlobType) {
> -    if (StrCmp (FileName, mKernelBlob[BlobType].Name) == 0) {
> -      break;
> -    }
> -  }
> -  if (BlobType == KernelBlobTypeMax) {
> -    return EFI_NOT_FOUND;
> -  }
> -
> -  //
> -  // Found it.
> -  //
> -  NewStubFile = AllocatePool (sizeof *NewStubFile);
> -  if (NewStubFile == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  NewStubFile->Signature = STUB_FILE_SIG;
> -  NewStubFile->BlobType  = (KERNEL_BLOB_TYPE)BlobType;
> -  NewStubFile->Position  = 0;
> -  CopyMem (&NewStubFile->File, &mEfiFileProtocolTemplate,
> -    sizeof mEfiFileProtocolTemplate);
> -  *NewHandle = &NewStubFile->File;
> -
> -  return EFI_SUCCESS;
> -}
> -
>  
>  /**
>    Closes a specified file handle.
> @@ -797,6 +735,74 @@ STATIC CONST EFI_FILE_PROTOCOL mEfiFileProtocolTemplate = {
>    NULL                        // FlushEx, revision 2
>  };
>  
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +StubFileOpen (
> +  IN EFI_FILE_PROTOCOL  *This,
> +  OUT EFI_FILE_PROTOCOL **NewHandle,
> +  IN CHAR16             *FileName,
> +  IN UINT64             OpenMode,
> +  IN UINT64             Attributes
> +  )
> +{
> +  CONST STUB_FILE *StubFile;
> +  UINTN           BlobType;
> +  STUB_FILE       *NewStubFile;
> +
> +  //
> +  // We're read-only.
> +  //
> +  switch (OpenMode) {
> +    case EFI_FILE_MODE_READ:
> +      break;
> +
> +    case EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE:
> +    case EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE:
> +      return EFI_WRITE_PROTECTED;
> +
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Only the root directory supports opening files in it.
> +  //
> +  StubFile = STUB_FILE_FROM_FILE (This);
> +  if (StubFile->BlobType != KernelBlobTypeMax) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Locate the file.
> +  //
> +  for (BlobType = 0; BlobType < KernelBlobTypeMax; ++BlobType) {
> +    if (StrCmp (FileName, mKernelBlob[BlobType].Name) == 0) {
> +      break;
> +    }
> +  }
> +  if (BlobType == KernelBlobTypeMax) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  //
> +  // Found it.
> +  //
> +  NewStubFile = AllocatePool (sizeof *NewStubFile);
> +  if (NewStubFile == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  NewStubFile->Signature = STUB_FILE_SIG;
> +  NewStubFile->BlobType  = (KERNEL_BLOB_TYPE)BlobType;
> +  NewStubFile->Position  = 0;
> +  CopyMem (&NewStubFile->File, &mEfiFileProtocolTemplate,
> +    sizeof mEfiFileProtocolTemplate);
> +  *NewHandle = &NewStubFile->File;
> +
> +  return EFI_SUCCESS;
> +}
> +
>  
>  //
>  // Protocol member functions for SimpleFileSystem.
>