From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.15134.1583511275672598075 for ; Fri, 06 Mar 2020 08:14:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MC/x8tQW; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583511274; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=e8Cj4j1KXmodE/m8w3j1SZ6DfbGdn4PDBT8yAJj5YoQ=; b=MC/x8tQWpcqAJneeRSTDxyqKrIV6Oc47TKw09qcK2XqdmpkzYc4cJ/FwlPEuMTWtfnFtdK CWIF7dttZfEthb56yrBHtQF3Al9C24AuDXxrw7U0PvjIddBuQ31DQJ92VBNtjJ5SIW+Bhk 19O4fSazOQh6JaTSov9T7oWPOg8stJw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-253-68QArK5_MGKwZUcNqyZd-A-1; Fri, 06 Mar 2020 11:14:33 -0500 X-MC-Unique: 68QArK5_MGKwZUcNqyZd-A-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4C4341005510; Fri, 6 Mar 2020 16:14:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-130.ams2.redhat.com [10.36.117.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3F88310027A4; Fri, 6 Mar 2020 16:14:31 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg/QemuKernelLoaderFsDxe: drop tentative const object definition To: Ard Biesheuvel , devel@edk2.groups.io Cc: "Feng, Bob C" References: <20200306073841.13528-1-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: Date: Fri, 6 Mar 2020 17:14:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200306073841.13528-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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" > Signed-off-by: Ard Biesheuvel > --- > 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 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. >