From 157a222a2001e3d5df4851ea3f43a00873f82be1 Mon Sep 17 00:00:00 2001
Message-Id: <157a222a2001e3d5df4851ea3f43a00873f82be1@dist-git>
From: Michal Privoznik <mprivozn@redhat.com>
Date: Tue, 25 Feb 2020 11:24:50 +0100
Subject: [PATCH] virSecurityManagerMetadataLock: Store locked paths

So far, in the lock state we are storing only the file
descriptors of the files we've locked. Therefore, when unlocking
them and something does wrong the only thing we can report is FD
number, which is not user friendly at all. But if we store paths
among with FDs we can do better error reporting.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
(cherry picked from commit 256e01e59e922ff70dce56284e53e3463d4dc072)

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1804672

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Message-Id: <8b12c9adde3c8553b4a2b588ac1a657d0638336b.1582626185.git.mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/security/security_manager.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index f229d94570..05d20e36af 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1246,8 +1246,9 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
 
 
 struct _virSecurityManagerMetadataLockState {
-    size_t nfds;
+    size_t nfds; /* Captures size of both @fds and @paths */
     int *fds;
+    const char **paths;
 };
 
 
@@ -1278,6 +1279,7 @@ cmpstringp(const void *p1, const void *p2)
  *
  * Lock passed @paths for metadata change. The returned state
  * should be passed to virSecurityManagerMetadataUnlock.
+ * Passed @paths must not be freed until the corresponding unlock call.
  *
  * NOTE: this function is not thread safe (because of usage of
  * POSIX locks).
@@ -1293,9 +1295,11 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr G_GNUC_UNUSED,
     size_t i = 0;
     size_t nfds = 0;
     int *fds = NULL;
+    const char **locked_paths = NULL;
     virSecurityManagerMetadataLockStatePtr ret = NULL;
 
-    if (VIR_ALLOC_N(fds, npaths) < 0)
+    if (VIR_ALLOC_N(fds, npaths) < 0 ||
+        VIR_ALLOC_N(locked_paths, npaths) < 0)
         return NULL;
 
     /* Sort paths to lock in order to avoid deadlocks with other
@@ -1372,12 +1376,14 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr G_GNUC_UNUSED,
             break;
         } while (1);
 
+        locked_paths[nfds] = p;
         VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
     }
 
     if (VIR_ALLOC(ret) < 0)
         goto cleanup;
 
+    ret->paths = g_steal_pointer(&locked_paths);
     ret->fds = g_steal_pointer(&fds);
     ret->nfds = nfds;
     nfds = 0;
@@ -1386,6 +1392,7 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr G_GNUC_UNUSED,
     for (i = nfds; i > 0; i--)
         VIR_FORCE_CLOSE(fds[i - 1]);
     VIR_FREE(fds);
+    VIR_FREE(locked_paths);
     return ret;
 }
 
@@ -1401,21 +1408,23 @@ virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr G_GNUC_UNUSED,
 
     for (i = 0; i < (*state)->nfds; i++) {
         char ebuf[1024];
+        const char *path = (*state)->paths[i];
         int fd = (*state)->fds[i];
 
         /* Technically, unlock is not needed because it will
          * happen on VIR_CLOSE() anyway. But let's play it nice. */
         if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) {
-            VIR_WARN("Unable to unlock fd %d: %s",
-                     fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+            VIR_WARN("Unable to unlock fd %d path %s: %s",
+                     fd, path, virStrerror(errno, ebuf, sizeof(ebuf)));
         }
 
         if (VIR_CLOSE(fd) < 0) {
-            VIR_WARN("Unable to close fd %d: %s",
-                     fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+            VIR_WARN("Unable to close fd %d path %s: %s",
+                     fd, path, virStrerror(errno, ebuf, sizeof(ebuf)));
         }
     }
 
     VIR_FREE((*state)->fds);
+    VIR_FREE((*state)->paths);
     VIR_FREE(*state);
 }
-- 
2.25.1