* [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)
@ 2025-11-01 13:14 Michael Tokarev
2025-11-01 23:29 ` Rodrigo Dias Correa
2025-11-02 9:24 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 5+ messages in thread
From: Michael Tokarev @ 2025-11-01 13:14 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Tokarev, qemu-trivial, Rodrigo Dias Correa,
Kostiantyn Kostiuk
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>
---
v3: and actually don't forget to commit the changes.
I'm sorry for the noize.
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..7785150fe4 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) == 0) {
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) == 0) {
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) == 0) {
shutdown_cmd = REBOOT_CMD_PATH;
}
shutdown_flag = reboot_flag;
--
2.47.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)
2025-11-01 13:14 [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2) Michael Tokarev
@ 2025-11-01 23:29 ` Rodrigo Dias Correa
2025-11-02 10:12 ` Michael Tokarev
2025-11-02 9:24 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 5+ messages in thread
From: Rodrigo Dias Correa @ 2025-11-01 23:29 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel; +Cc: qemu-trivial, Kostiantyn Kostiuk
On Sat Nov 1, 2025 at 2:14 PM CET, Michael Tokarev wrote:
> 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>
Reviewed-by: Rodrigo Dias Correa <r@drigo.nl>
Thanks for fixing it.
Rodrigo
> ---
> v3: and actually don't forget to commit the changes.
> I'm sorry for the noize.
> 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..7785150fe4 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) == 0) {
> 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) == 0) {
> 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) == 0) {
> shutdown_cmd = REBOOT_CMD_PATH;
> }
> shutdown_flag = reboot_flag;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)
2025-11-01 13:14 [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2) Michael Tokarev
2025-11-01 23:29 ` Rodrigo Dias Correa
@ 2025-11-02 9:24 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-11-02 9:24 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel
Cc: qemu-trivial, Rodrigo Dias Correa, Kostiantyn Kostiuk
On 1/11/25 14:14, Michael Tokarev wrote:
> 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>
> ---
> v3: and actually don't forget to commit the changes.
> I'm sorry for the noize.
> 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(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)
2025-11-01 23:29 ` Rodrigo Dias Correa
@ 2025-11-02 10:12 ` Michael Tokarev
2025-11-03 8:00 ` Kostiantyn Kostiuk
0 siblings, 1 reply; 5+ messages in thread
From: Michael Tokarev @ 2025-11-02 10:12 UTC (permalink / raw)
To: Rodrigo Dias Correa, qemu-devel; +Cc: qemu-trivial, Kostiantyn Kostiuk
On 11/2/25 02:29, Rodrigo Dias Correa wrote:
..> Thanks for fixing it.
Hi!
It wasn't really broken per se. Just confusing.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)
2025-11-02 10:12 ` Michael Tokarev
@ 2025-11-03 8:00 ` Kostiantyn Kostiuk
0 siblings, 0 replies; 5+ messages in thread
From: Kostiantyn Kostiuk @ 2025-11-03 8:00 UTC (permalink / raw)
To: Michael Tokarev; +Cc: Rodrigo Dias Correa, qemu-devel, qemu-trivial
[-- Attachment #1: Type: text/plain, Size: 347 bytes --]
Reviewed-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Best Regards,
Kostiantyn Kostiuk.
On Sun, Nov 2, 2025 at 12:15 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> On 11/2/25 02:29, Rodrigo Dias Correa wrote:
> ..> Thanks for fixing it.
>
> Hi!
>
> It wasn't really broken per se. Just confusing.
>
> Thanks,
>
> /mjt
>
>
[-- Attachment #2: Type: text/html, Size: 888 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-11-03 8:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-01 13:14 [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2) Michael Tokarev
2025-11-01 23:29 ` Rodrigo Dias Correa
2025-11-02 10:12 ` Michael Tokarev
2025-11-03 8:00 ` Kostiantyn Kostiuk
2025-11-02 9:24 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).