OvmfPkg/VirtioFsDxe: flush, sync, release and forget in Close() / Delete()

The two member functions that free the EFI_FILE_PROTOCOL object are
Close() and Delete(). Before we create VIRTIO_FS_FILE objects with
EFI_FILE_PROTOCOL.Open() later in this patch series, extend each of these
"destructor" functions to get rid of the FuseHandle and NodeId resources
properly -- in a way that matches each function's own purpose.

For the time being, VirtioFsSimpleFileDelete() only gets a reminder about
its core task (namely, removing the file), as the needed machinery will
become only later. But we can already outline the "task list", and deal
with the FuseHandle and NodeId resources. The "task list" of
VirtioFsSimpleFileDelete() is different from that of
VirtioFsSimpleFileClose(), thus both destructors diverge at this point.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3097
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20201216211125.19496-16-lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsClose.c b/OvmfPkg/VirtioFsDxe/SimpleFsClose.c
index 01bbeae2147397f179e1099abf5a76944a7fdd46..bc91ad726b2c768a98dab729720600b9b6d3b1b0 100644 (file)
--- a/OvmfPkg/VirtioFsDxe/SimpleFsClose.c
+++ b/OvmfPkg/VirtioFsDxe/SimpleFsClose.c
@@ -24,20 +24,36 @@ VirtioFsSimpleFileClose (
   VirtioFs     = VirtioFsFile->OwnerFs;\r
 \r
   //\r
-  // At this point, the implementation is only suitable for closing the\r
-  // VIRTIO_FS_FILE that was created by VirtioFsOpenVolume().\r
+  // All actions in this function are "best effort"; the UEFI spec requires\r
+  // EFI_FILE_PROTOCOL.Close() to sync all data to the device, but it also\r
+  // requires EFI_FILE_PROTOCOL.Close() to release resources unconditionally,\r
+  // and to return EFI_SUCCESS unconditionally.\r
   //\r
-  ASSERT (VirtioFsFile->IsDirectory);\r
-  ASSERT (VirtioFsFile->NodeId == VIRTIO_FS_FUSE_ROOT_DIR_NODE_ID);\r
-  //\r
-  // Close the root directory.\r
-  //\r
-  // Ignore any errors, because EFI_FILE_PROTOCOL.Close() is required to\r
-  // release the EFI_FILE_PROTOCOL object unconditionally.\r
+  // Flush, sync, release, and (if needed) forget. If any action fails, we\r
+  // still try the others.\r
   //\r
+  if (VirtioFsFile->IsOpenForWriting) {\r
+    if (!VirtioFsFile->IsDirectory) {\r
+      VirtioFsFuseFlush (VirtioFs, VirtioFsFile->NodeId,\r
+        VirtioFsFile->FuseHandle);\r
+    }\r
+\r
+    VirtioFsFuseFsyncFileOrDir (VirtioFs, VirtioFsFile->NodeId,\r
+      VirtioFsFile->FuseHandle, VirtioFsFile->IsDirectory);\r
+  }\r
+\r
   VirtioFsFuseReleaseFileOrDir (VirtioFs, VirtioFsFile->NodeId,\r
     VirtioFsFile->FuseHandle, VirtioFsFile->IsDirectory);\r
 \r
+  //\r
+  // VirtioFsFile->FuseHandle is gone at this point, but VirtioFsFile->NodeId\r
+  // is still valid. If we've known VirtioFsFile->NodeId from a lookup, then\r
+  // now we should ask the server to forget it *once*.\r
+  //\r
+  if (VirtioFsFile->NodeId != VIRTIO_FS_FUSE_ROOT_DIR_NODE_ID) {\r
+    VirtioFsFuseForget (VirtioFs, VirtioFsFile->NodeId);\r
+  }\r
+\r
   //\r
   // One fewer file left open for the owner filesystem.\r
   //\r

diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsDelete.c b/OvmfPkg/VirtioFsDxe/SimpleFsDelete.c
index 3209923d1e495fe252f687e5c7b891cb7b27602b..bbad64bf788674fb7fbeece292961f02aab5c587 100644 (file)
--- a/OvmfPkg/VirtioFsDxe/SimpleFsDelete.c
+++ b/OvmfPkg/VirtioFsDxe/SimpleFsDelete.c
@@ -6,6 +6,9 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent\r
 **/\r
 \r
+#include <Library/BaseLib.h>             // RemoveEntryList()\r
+#include <Library/MemoryAllocationLib.h> // FreePool()\r
+\r
 #include "VirtioFsDxe.h"\r
 \r
 EFI_STATUS\r
@@ -14,16 +17,52 @@ VirtioFsSimpleFileDelete (
   IN EFI_FILE_PROTOCOL *This\r
   )\r
 {\r
+  VIRTIO_FS_FILE *VirtioFsFile;\r
+  VIRTIO_FS      *VirtioFs;\r
+  EFI_STATUS     Status;\r
+\r
+  VirtioFsFile = VIRTIO_FS_FILE_FROM_SIMPLE_FILE (This);\r
+  VirtioFs     = VirtioFsFile->OwnerFs;\r
+\r
+  //\r
+  // All actions in this function are "best effort"; the UEFI spec requires\r
+  // EFI_FILE_PROTOCOL.Delete() to release resources unconditionally. If a step\r
+  // related to removing the file fails, it's only reflected in the return\r
+  // status (EFI_WARN_DELETE_FAILURE rather than EFI_SUCCESS).\r
+  //\r
+  // Release, remove, and (if needed) forget. We don't waste time flushing and\r
+  // syncing; if the EFI_FILE_PROTOCOL user cares enough, they should keep the\r
+  // parent directory open until after this function call returns, and then\r
+  // force a sync on *that* EFI_FILE_PROTOCOL instance, using either the\r
+  // Flush() member function, or the Close() member function.\r
+  //\r
+  // If any action fails below, we still try the others.\r
+  //\r
+  VirtioFsFuseReleaseFileOrDir (VirtioFs, VirtioFsFile->NodeId,\r
+    VirtioFsFile->FuseHandle, VirtioFsFile->IsDirectory);\r
+\r
+  //\r
+  // VirtioFsFile->FuseHandle is gone at this point, but VirtioFsFile->NodeId\r
+  // is still valid. Continue with removing the file or directory. The result\r
+  // of this operation determines the return status of the function.\r
   //\r
-  // At this point, the implementation is only suitable for closing the\r
-  // VIRTIO_FS_FILE that was created by VirtioFsOpenVolume().\r
+  // TODO\r
   //\r
-  // Actually deleting the root directory is not possible, so we're only going\r
-  // to release resources, and return EFI_WARN_DELETE_FAILURE.\r
+  Status = EFI_WARN_DELETE_FAILURE;\r
+\r
   //\r
-  // In order to release resources, VirtioFsSimpleFileClose() is just right\r
-  // here.\r
+  // Finally, if we've known VirtioFsFile->NodeId from a lookup, then we should\r
+  // also ask the server to forget it *once*.\r
   //\r
-  VirtioFsSimpleFileClose (This);\r
-  return EFI_WARN_DELETE_FAILURE;\r
+  if (VirtioFsFile->NodeId != VIRTIO_FS_FUSE_ROOT_DIR_NODE_ID) {\r
+    VirtioFsFuseForget (VirtioFs, VirtioFsFile->NodeId);\r
+  }\r
+\r
+  //\r
+  // One fewer file left open for the owner filesystem.\r
+  //\r
+  RemoveEntryList (&VirtioFsFile->OpenFilesEntry);\r
+\r
+  FreePool (VirtioFsFile);\r
+  return Status;\r
 }\r

diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpenVolume.c b/OvmfPkg/VirtioFsDxe/SimpleFsOpenVolume.c
index 8c1457a68aaddfd6ec8ab3456d7465b13482d41a..67d2deb6bdf27ee0fadb98ddf3c7902349eeed8a 100644 (file)
--- a/OvmfPkg/VirtioFsDxe/SimpleFsOpenVolume.c
+++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpenVolume.c
@@ -62,6 +62,7 @@ VirtioFsOpenVolume (
   VirtioFsFile->SimpleFile.SetInfo     = VirtioFsSimpleFileSetInfo;\r
   VirtioFsFile->SimpleFile.Flush       = VirtioFsSimpleFileFlush;\r
   VirtioFsFile->IsDirectory            = TRUE;\r
+  VirtioFsFile->IsOpenForWriting       = FALSE;\r
   VirtioFsFile->OwnerFs                = VirtioFs;\r
   VirtioFsFile->NodeId                 = VIRTIO_FS_FUSE_ROOT_DIR_NODE_ID;\r
   VirtioFsFile->FuseHandle             = RootDirHandle;\r

diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
index 7151a62bb42b8d023d082912bf40ea741138e72f..1cbd27d8fb524e694464252ac9bb44645ec6a8e1 100644 (file)
--- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
+++ b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
@@ -110,6 +110,7 @@ typedef struct {
   UINT64            Signature;\r
   EFI_FILE_PROTOCOL SimpleFile;\r
   BOOLEAN           IsDirectory;\r
+  BOOLEAN           IsOpenForWriting;\r
   VIRTIO_FS         *OwnerFs;\r
   LIST_ENTRY        OpenFilesEntry;\r
   //\r