From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web10.15449.1583512855704425874 for ; Fri, 06 Mar 2020 08:40:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=FwitUfH2; spf=pass (domain: linaro.org, ip: 209.85.128.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f67.google.com with SMTP id i14so3075188wmb.1 for ; Fri, 06 Mar 2020 08:40:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dl7nS6SwkbbImHTHiDGy6rsIjibx/dIkx377K08hugg=; b=FwitUfH2UyiVLrFowiyrvOX0gx+QPIu/c0CYFDWIlMwesIu7vBitmP36qob5vi62n4 hJsNW/soVYsYjKD52mywx1rMjWx7cBWDD+FFcMdk3VApXTlv9ZKgxa5yu1egEW/XrPhd 3aGojUPp9tWvWMiB+K0cfFOdH8Lv4mfU4xKo0btsOPdA3ftvsZFVzCLkPaOExN82lNS/ K1FL6Ak5RTqwQQiRDAVE6TqNimKYdaG1kUgsnU1jCxV6HOB3L1hnJzDPHGn3L0zDD1k2 K9d8IK1UYwYH0pVXIHcsHnFGj0WcE0TXpvc9Nq2YAZuSvbYp/mJp/KLNv5NJNp/5vE/p L4BA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dl7nS6SwkbbImHTHiDGy6rsIjibx/dIkx377K08hugg=; b=SKASi87T2UFonJgmzopKm/Oq7XRM47JlnzfigM6ZHc31gP6m+4Lo46ozGO3ck6AtJ2 56JaOJaKBhUH2UI/AowVpMNsacudKDeqHgYPiZLbHYPT6XsQh1EcbiaeL9/1y3Ggg3vc h9AMimlddtY0OS7HFvO1Yet0uTjc1WOILTgcBF3ap1FGo9jmwdHAkmvAWrj9GM7Y4BIV BvMycEIW7jdF6K7cCy2EzpEWFj9HWQJFmFLUXyUryg1cOct/JSjO4O+wmVM5+NSF0svi mRdJisZk9jC4eBMjM2temHOLYqk9AdQGcAHHoXDpxcHJGAODaIGZL5aQQY4c2lmsSdn7 OxZg== X-Gm-Message-State: ANhLgQ1IiqmNVdLedebK25NQjKVlWkOHb2SQAu1fn8q0RFuugo8K6JrT cOhMStJjB4w34xcSpoxo90xLJl7A9KIY0AjBa7DtFg== X-Google-Smtp-Source: ADFU+vtqp0vddjzI6YxLWFVya+50aMgqZC8U7zDI3Ax9wcM7TeNfw10zdgwQuj72n2qp1Zwgu9Yl2becNRoQxgJJJhQ= X-Received: by 2002:a7b:c354:: with SMTP id l20mr3918031wmj.40.1583512853263; Fri, 06 Mar 2020 08:40:53 -0800 (PST) MIME-Version: 1.0 References: <20200306073841.13528-1-ard.biesheuvel@linaro.org> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 6 Mar 2020 17:40:42 +0100 Message-ID: Subject: Re: [PATCH] OvmfPkg/QemuKernelLoaderFsDxe: drop tentative const object definition To: Laszlo Ersek Cc: edk2-devel-groups-io , "Feng, Bob C" Content-Type: text/plain; charset="UTF-8" On Fri, 6 Mar 2020 at 17:14, Laszlo Ersek 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" > > 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. > > >