On Fri, 6 Mar 2020 at 17:14, Laszlo Ersek <ler...@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.f...@intel.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheu...@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 <ler...@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.
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55606): https://edk2.groups.io/g/devel/message/55606
Mute This Topic: https://groups.io/mt/71768124/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
