[libvirt] [PATCH 0/8] Remove use of 'areadlink' gnulib module (kill-a-gnulib-module-a-day initiative)
This removes use of 'areadlink' by using 'g_file_read_link' and fixes a few insane uses. Note that I'll post the actual module deletion from bootstrap.conf separately as touching that file results in lengthy rebuilds. Peter Krempa (8): qemu: domain: Use g_file_read_link instead of virFileReadLink util: file: Remove virFileReadLink qemu: tpm: Use g_autofree in qemuTPMEmulatorGetPid qemu: tpm: Sanitize error values in qemuTPMEmulatorGetPid qemu: gpu: Sanitize error values in qemuVhostUserGPUGetPid util: pidfile: Sanitize return values of virPidFileReadIfAlive util: pidfile: Sanitize return values of virPidFileReadPathIfAlive util: pidfile: Replace 'areadlink' by 'g_file_read_link' src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 18 +++++----- src/qemu/qemu_tpm.c | 16 ++++----- src/qemu/qemu_vhost_user_gpu.c | 12 ++++--- src/util/virfile.c | 13 ------- src/util/virfile.h | 3 -- src/util/virpidfile.c | 64 +++++++++++++++++----------------- 7 files changed, 56 insertions(+), 71 deletions(-) -- 2.23.0
In an effort to remove as much gnulib usage as possible let's reimplement virFileReadLink. Since it's used in two places only I opted to open-code it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 842b70dc26..40a617845a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13170,12 +13170,13 @@ qemuDomainCreateDeviceRecursive(const char *device, } if (isLink) { + g_autoptr(GError) gerr = NULL; + /* We are dealing with a symlink. Create a dangling symlink and descend * down one level which hopefully creates the symlink's target. */ - if (virFileReadLink(device, &target) < 0) { - virReportSystemError(errno, - _("unable to resolve symlink %s"), - device); + if (!(target = g_file_read_link(device, &gerr))) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to resolve symlink %s: %s"), device, gerr->message); goto cleanup; } @@ -14164,10 +14165,11 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, data.target = target; } else if (isLink) { - if (virFileReadLink(file, &target) < 0) { - virReportSystemError(errno, - _("unable to resolve symlink %s"), - file); + g_autoptr(GError) gerr = NULL; + + if (!(target = g_file_read_link(file, &gerr))) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to resolve symlink %s: %s"), file, gerr->message); return ret; } -- 2.23.0
The function is unused so we can remove it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virfile.c | 13 ------------- src/util/virfile.h | 3 --- 3 files changed, 17 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4d0d03c580..c706553e37 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1978,7 +1978,6 @@ virFileReadBufQuiet; virFileReadHeaderFD; virFileReadHeaderQuiet; virFileReadLimFD; -virFileReadLink; virFileReadValueBitmap; virFileReadValueInt; virFileReadValueScaledInt; diff --git a/src/util/virfile.c b/src/util/virfile.c index ced0ea70b7..b1aeaa5851 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -81,7 +81,6 @@ #include "virutil.h" #include "c-ctype.h" -#include "areadlink.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -1625,18 +1624,6 @@ virFileIsLink(const char *linkpath) return S_ISLNK(st.st_mode) != 0; } -/* - * Read where symlink is pointing to. - * - * Returns 0 on success (@linkpath is a successfully read link), - * -1 with errno set upon error. - */ -int -virFileReadLink(const char *linkpath, char **resultpath) -{ - return (*resultpath = areadlink(linkpath)) ? 0 : -1; -} - /* * Finds a requested executable file in the PATH env. e.g.: * "qemu-img" will return "/usr/bin/qemu-img" diff --git a/src/util/virfile.h b/src/util/virfile.h index a570529330..9a8709b52c 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -176,9 +176,6 @@ int virFileResolveAllLinks(const char *linkpath, int virFileIsLink(const char *linkpath) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; -int virFileReadLink(const char *linkpath, char **resultpath) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; - char *virFindFileInPath(const char *file); char *virFileFindResource(const char *filename, -- 2.23.0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 62b9f803bd..40136c4410 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -277,15 +277,13 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, { int ret; g_autofree char *swtpm = virTPMGetSwtpm(); - char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, - shortName); + g_autofree char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, + shortName); if (!pidfile) return -ENOMEM; ret = virPidFileReadPathIfAlive(pidfile, pid, swtpm); - VIR_FREE(pidfile); - return ret; } -- 2.23.0
The callers don't care about the actual return value, so return -1 rather than errno. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 40136c4410..2f92542470 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -266,7 +266,7 @@ qemuTPMEmulatorCreatePidFilename(const char *swtpmStateDir, * @shortName: short name of the domain * @pid: pointer to pid * - * Return -errno upon error, or zero on successful reading of the pidfile. + * Return -1 upon error, or zero on successful reading of the pidfile. * If the PID was not still alive, zero will be returned, and @pid will be * set to -1; */ @@ -275,16 +275,16 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, const char *shortName, pid_t *pid) { - int ret; g_autofree char *swtpm = virTPMGetSwtpm(); g_autofree char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName); if (!pidfile) - return -ENOMEM; + return -1; - ret = virPidFileReadPathIfAlive(pidfile, pid, swtpm); + if (virPidFileReadPathIfAlive(pidfile, pid, swtpm) < 0) + return -1; - return ret; + return 0; } -- 2.23.0
The caller doesn't care about the actual return value, so return -1 rather than errno. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_vhost_user_gpu.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c index 37e424eac3..51244f9b35 100644 --- a/src/qemu/qemu_vhost_user_gpu.c +++ b/src/qemu/qemu_vhost_user_gpu.c @@ -61,7 +61,7 @@ qemuVhostUserGPUCreatePidFilename(const char *stateDir, * @alias: video device alias * @pid: pointer to pid * - * Return -errno upon error, or zero on successful reading of the pidfile. + * Return -1 upon error, or zero on successful reading of the pidfile. * If the PID was not still alive, zero will be returned, and @pid will be * set to -1; */ @@ -74,11 +74,13 @@ qemuVhostUserGPUGetPid(const char *binPath, { g_autofree char *pidfile = NULL; - pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias); - if (!pidfile) - return -ENOMEM; + if (!(pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias))) + return -1; + + if (virPidFileReadPathIfAlive(pidfile, pid, binPath) < 0) + return -1; - return virPidFileReadPathIfAlive(pidfile, pid, binPath); + return 0; } -- 2.23.0
Return -1 on failure rather than -errno since none of the callers actually cares about the return value. This specifically fixes returns of -ENOMEM in cases of bad usage, which would report wrong error anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpidfile.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 3e78735c9c..b1b8e54993 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -282,7 +282,7 @@ int virPidFileReadPathIfAlive(const char *path, * and its executable path resolves to @binpath. This adds * protection against recycling of previously reaped pids. * - * Returns -errno upon error, or zero on successful + * Returns -1 upon error, or zero on successful * reading of the pidfile. If the PID was not still * alive, zero will be returned, but @pid will be * set to -1. @@ -295,12 +295,15 @@ int virPidFileReadIfAlive(const char *dir, g_autofree char *pidfile = NULL; if (name == NULL || dir == NULL) - return -EINVAL; + return -1; if (!(pidfile = virPidFileBuildPath(dir, name))) - return -ENOMEM; + return -1; + + if (virPidFileReadPathIfAlive(pidfile, pid, binpath) < 0) + return -1; - return virPidFileReadPathIfAlive(pidfile, pid, binpath); + return 0; } -- 2.23.0
The callers don't actually use the returned errno for reporting errors. Additionally virFileResolveAllLinks returns -1 rather than -errno on error thus you'd get a spurious EPERM even on other errors. Don't try to return errno in this case. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpidfile.c | 50 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index b1b8e54993..4a800b6528 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -182,7 +182,7 @@ int virPidFileRead(const char *dir, * If @binpath is NULL the check for the executable path * is skipped. * - * Returns -errno upon error, or zero on successful + * Returns -1 upon error, or zero on successful * reading of the pidfile. If the PID was not still * alive, zero will be returned, but @pid will be * set to -1. @@ -191,8 +191,8 @@ int virPidFileReadPathIfAlive(const char *path, pid_t *pid, const char *binPath) { - int ret; - bool isLink; + int rc; + bool isLink = false; size_t procLinkLen; const char deletedText[] = " (deleted)"; size_t deletedTextLen = strlen(deletedText); @@ -205,16 +205,15 @@ int virPidFileReadPathIfAlive(const char *path, /* only set this at the very end on success */ *pid = -1; - if ((ret = virPidFileReadPath(path, &retPid)) < 0) - return ret; + if (virPidFileReadPath(path, &retPid) < 0) + return -1; #ifndef WIN32 /* Check that it's still alive. Safe to skip this sanity check on * mingw, which lacks kill(). */ if (kill(retPid, 0) < 0) { - ret = 0; - retPid = -1; - goto cleanup; + *pid = -1; + return 0; } #endif @@ -222,23 +221,24 @@ int virPidFileReadPathIfAlive(const char *path, /* we only knew the pid, and that pid is alive, so we can * return it. */ - ret = 0; - goto cleanup; + *pid = retPid; + return 0; } procPath = g_strdup_printf("/proc/%lld/exe", (long long)retPid); - if ((ret = virFileIsLink(procPath)) < 0) - return ret; + if ((rc = virFileIsLink(procPath)) < 0) + return -1; - isLink = ret; + if (rc == 1) + isLink = true; if (isLink && virFileLinkPointsTo(procPath, binPath)) { /* the link in /proc/$pid/exe is a symlink to a file * that has the same inode as the file at binpath. */ - ret = 0; - goto cleanup; + *pid = retPid; + return 0; } /* Even if virFileLinkPointsTo returns a mismatch, it could be @@ -248,24 +248,22 @@ int virPidFileReadPathIfAlive(const char *path, * part, and see if it has the same canonicalized name as binpath. */ if (!(procLink = areadlink(procPath))) - return -errno; + return -1; procLinkLen = strlen(procLink); if (procLinkLen > deletedTextLen) procLink[procLinkLen - deletedTextLen] = 0; - if ((ret = virFileResolveAllLinks(binPath, &resolvedBinPath)) < 0) - return ret; - if ((ret = virFileResolveAllLinks(procLink, &resolvedProcLink)) < 0) - return ret; + if (virFileResolveAllLinks(binPath, &resolvedBinPath) < 0) + return -1; + if (virFileResolveAllLinks(procLink, &resolvedProcLink) < 0) + return -1; - ret = STREQ(resolvedBinPath, resolvedProcLink) ? 0 : -1; + if (STRNEQ(resolvedBinPath, resolvedProcLink)) + return -1; - cleanup: - /* return the originally set pid of -1 unless we proclaim success */ - if (ret == 0) - *pid = retPid; - return ret; + *pid = retPid; + return 0; } -- 2.23.0
Use the glib function rather than gnulib. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpidfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 4a800b6528..5d1a820cd1 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -35,7 +35,6 @@ #include "virlog.h" #include "virerror.h" #include "c-ctype.h" -#include "areadlink.h" #include "virstring.h" #include "virprocess.h" @@ -247,7 +246,7 @@ int virPidFileReadPathIfAlive(const char *path, * "$procpath (deleted)". Read that link, remove the " (deleted)" * part, and see if it has the same canonicalized name as binpath. */ - if (!(procLink = areadlink(procPath))) + if (!(procLink = g_file_read_link(procPath, NULL))) return -1; procLinkLen = strlen(procLink); -- 2.23.0
On Wed, Nov 13, 2019 at 14:07:01 +0100, Peter Krempa wrote:
This removes use of 'areadlink' by using 'g_file_read_link' and fixes a few insane uses.
Note that I'll post the actual module deletion from bootstrap.conf separately as touching that file results in lengthy rebuilds.
The commits actually deleting the modules from bootstrap.conf will be agreggated here: https://gitlab.com/pipo.sk/libvirt/commits/gnulib
On Wed, Nov 13, 2019 at 02:07:01PM +0100, Peter Krempa wrote:
This removes use of 'areadlink' by using 'g_file_read_link' and fixes a few insane uses.
Note that I'll post the actual module deletion from bootstrap.conf separately as touching that file results in lengthy rebuilds.
Peter Krempa (8): qemu: domain: Use g_file_read_link instead of virFileReadLink util: file: Remove virFileReadLink qemu: tpm: Use g_autofree in qemuTPMEmulatorGetPid qemu: tpm: Sanitize error values in qemuTPMEmulatorGetPid qemu: gpu: Sanitize error values in qemuVhostUserGPUGetPid util: pidfile: Sanitize return values of virPidFileReadIfAlive util: pidfile: Sanitize return values of virPidFileReadPathIfAlive util: pidfile: Replace 'areadlink' by 'g_file_read_link'
src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 18 +++++----- src/qemu/qemu_tpm.c | 16 ++++----- src/qemu/qemu_vhost_user_gpu.c | 12 ++++--- src/util/virfile.c | 13 ------- src/util/virfile.h | 3 -- src/util/virpidfile.c | 64 +++++++++++++++++----------------- 7 files changed, 56 insertions(+), 71 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa