[v2] qga: use access(2) to check for command existance instead of questionable stat(2)

[PATCH trivial v2] qga: use access(2) to check for command existance instead of questionable stat(2)

Michael Tokarev posted 1 patch 1 week, 6 days ago

Diff against v1 v3
Download mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251101131250.792706-1-mjt@tls.msk.ru

Maintainers: Michael Roth <michael.roth@amd.com>, Kostiantyn Kostiuk <kkostiuk@redhat.com>, Michael Tokarev <mjt@tls.msk.ru>, Laurent Vivier <laurent@vivier.eu>

There is a newer version of this series

qga/commands-posix.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

Expand all Fold all

[PATCH trivial v2] qga: use access(2) to check for command existance instead of questionable stat(2)

Posted by Michael Tokarev 1 week, 6 days ago

The code checks existance of a command (halt/poweroff/reboot) by using
stat(2) and immediately checking for S_ISLNK() on the returned stat
struct.  This check will never be true, because stat(2) always follows
symbolic links and hence will either return ENOENT (in case of dangling
symlink) or the properties for the final target file.  It is lstat(2)
which might return information about the symlink itself.  However, even
there, we want to check the final file properties, not the first symlink.

This check - S_ISLNK - is harmful but useless in this case.  However, it
is confusing and it helps the wrong usage of stat(2) to spread, so it is
better to remove it.

Additionally, the code would better to check for the executable bits
of the final file, not check if it's a regular file - it's sort of
dubious to have anything but regular files in /sbin/.

But a POSIX system provides another command which suits the purpose
perfectly: it is access(2).  And it is so simple that it's not
necessary to create a separate function when usin it.

Replace stat(2) with access(X_OK) to check for file existance in
qga/commands-posix.c

Fixes: c5b4afd4d56e "qga: Support guest shutdown of BusyBox-based systems"
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
v2: fix reverse logic of the access() tests.
    I should write code more often :)

 qga/commands-posix.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c7059857e4..0f4d6d96cc 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -213,12 +213,6 @@ out:
     return retcode;
 }
 
-static bool file_exists(const char *path)
-{
-    struct stat st;
-    return stat(path, &st) == 0 && (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode));
-}
-
 #define POWEROFF_CMD_PATH "/sbin/poweroff"
 #define HALT_CMD_PATH "/sbin/halt"
 #define REBOOT_CMD_PATH "/sbin/reboot"
@@ -245,17 +239,17 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
 
     slog("guest-shutdown called, mode: %s", mode);
     if (!mode || strcmp(mode, "powerdown") == 0) {
-        if (file_exists(POWEROFF_CMD_PATH)) {
+        if (access(POWEROFF_CMD_PATH, X_OK)) {
             shutdown_cmd = POWEROFF_CMD_PATH;
         }
         shutdown_flag = powerdown_flag;
     } else if (strcmp(mode, "halt") == 0) {
-        if (file_exists(HALT_CMD_PATH)) {
+        if (access(HALT_CMD_PATH, X_OK)) {
             shutdown_cmd = HALT_CMD_PATH;
         }
         shutdown_flag = halt_flag;
     } else if (strcmp(mode, "reboot") == 0) {
-        if (file_exists(REBOOT_CMD_PATH)) {
+        if (access(REBOOT_CMD_PATH, X_OK)) {
             shutdown_cmd = REBOOT_CMD_PATH;
         }
         shutdown_flag = reboot_flag;
-- 
2.47.3