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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 6 Mar 2020 at 17:14, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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/
>

Are you sure? The const object has static linkage.

> > 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.
> >
>