From 157a222a2001e3d5df4851ea3f43a00873f82be1 Mon Sep 17 00:00:00 2001 Message-Id: <157a222a2001e3d5df4851ea3f43a00873f82be1@dist-git> From: Michal Privoznik 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 Reviewed-by: Peter Krempa (cherry picked from commit 256e01e59e922ff70dce56284e53e3463d4dc072) Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1804672 Signed-off-by: Michal Privoznik Message-Id: <8b12c9adde3c8553b4a2b588ac1a657d0638336b.1582626185.git.mprivozn@redhat.com> Reviewed-by: Jiri Denemark --- 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