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>

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index fe9def7fb9dde4e654adc1f19b2ec8c9e2f3e0ad..aea8cc2fb9671a69ec7078daf9f3c4ee1f11565e 100644 (file)
--- 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
@@ -1374,12 +1378,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;
@@ -1388,6 +1394,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;
 }
 
@@ -1403,21 +1410,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);
 }