[PATCH v2 0/7] qga/commands-posix: replace code duplicating commands with a helper

[PATCH v2 0/7] qga/commands-posix: replace code duplicating commands with a helper

[Andrey Drobyshev]

v1 --> v2:
 * Replace commit for guest-get-fsinfo: instead of reporting statvfs(3)
   values directly, keep the old ones but add an additional optional
   field 'total-bytes-root', only reported in POSIX.  Also tweak the
   fields description in qga/qapi-schema.json to document where the
   values come from.

v1: https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg05766.html

Andrey Drobyshev (7):
  qga: guest-get-fsinfo: add optional 'total-bytes-root' field
  qga: introduce ga_run_command() helper for guest cmd execution
  qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper
  qga/commands-posix: qmp_guest_set_time: use ga_run_command helper
  qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper
  qga/commands-posix: use ga_run_command helper when suspending via
    sysfs
  qga/commands-posix: qmp_guest_set_user_password: use ga_run_command
    helper

 qga/commands-posix.c | 389 +++++++++++++++++++------------------------
 qga/commands-win32.c |   1 +
 qga/qapi-schema.json |  12 +-
 3 files changed, 182 insertions(+), 220 deletions(-)

-- 
2.39.3

[PATCH v2 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field

[Andrey Drobyshev]

Since the commit 25b5ff1a86 ("qga: add mountpoint usage info to
GuestFilesystemInfo") we have 2 values reported in guest-get-fsinfo:
used = (f_blocks - f_bfree), total = (f_blocks - f_bfree + f_bavail) as
returned by statvfs(3).  While on Windows guests that's all we can get
with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
total file system size, as it's visible for root user.  Let's add an
optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
only be reported on POSIX and represent f_blocks value as returned by
statvfs(3).

While here, let's document better where those values come from in both
POSIX and Windows.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c |  2 ++
 qga/commands-win32.c |  1 +
 qga/qapi-schema.json | 12 +++++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26008db497..8207c4c47e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1569,8 +1569,10 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
         nonroot_total = used + buf.f_bavail;
         fs->used_bytes = used * fr_size;
         fs->total_bytes = nonroot_total * fr_size;
+        fs->total_bytes_root = buf.f_blocks * fr_size;
 
         fs->has_total_bytes = true;
+        fs->has_total_bytes_root = true;
         fs->has_used_bytes = true;
     }
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a1015757d8..9e820aad8d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1143,6 +1143,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
     fs = g_malloc(sizeof(*fs));
     fs->name = g_strdup(guid);
     fs->has_total_bytes = false;
+    fs->has_total_bytes_root = false;
     fs->has_used_bytes = false;
     if (len == 0) {
         fs->mountpoint = g_strdup("System Reserved");
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b8efe31897..093a5ab602 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1031,8 +1031,18 @@
 # @type: file system type string
 #
 # @used-bytes: file system used bytes (since 3.0)
+#     * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
+#     * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
+#       by GetDiskFreeSpaceEx()
 #
 # @total-bytes: non-root file system total bytes (since 3.0)
+#     * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
+#       statvfs(3)
+#     * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
+#
+# @total-bytes-root: total file system size in bytes (as visible for a
+#     priviledged user) (since 8.3)
+#     * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
 #
 # @disk: an array of disk hardware information that the volume lies
 #     on, which may be empty if the disk type is not supported
@@ -1042,7 +1052,7 @@
 { 'struct': 'GuestFilesystemInfo',
   'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
            '*used-bytes': 'uint64', '*total-bytes': 'uint64',
-           'disk': ['GuestDiskAddress']} }
+           '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
 
 ##
 # @guest-get-fsinfo:
-- 
2.39.3

[PATCH v2 2/7] qga: introduce ga_run_command() helper for guest cmd execution

[Andrey Drobyshev]

When executing guest commands in *nix environment, we repeat the same
fork/exec pattern multiple times.  Let's just separate it into a single
helper which would also be able to feed input data into the launched
process' stdin.  This way we can avoid code duplication.

To keep the history more bisectable, let's replace qmp commands
implementations one by one.  Also add G_GNUC_UNUSED attribute to the
helper and remove it in the next commit.

Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 140 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8207c4c47e..781498418f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -76,6 +76,146 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
     g_assert(rpid == pid);
 }
 
+static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
+{
+    ssize_t n;
+    char buf[1024];
+    close(fd[1]);
+    fd[1] = -1;
+    while ((n = read(fd[0], buf, sizeof(buf))) != 0) {
+        if (n < 0) {
+            if (errno == EINTR) {
+                continue;
+            } else {
+                break;
+            }
+        }
+        *str = g_realloc(*str, *len + n);
+        memcpy(*str + *len, buf, n);
+        *len += n;
+    }
+    close(fd[0]);
+    fd[0] = -1;
+}
+
+/*
+ * Helper to run command with input/output redirection,
+ * sending string to stdin and taking error message from
+ * stdout/err.
+ */
+G_GNUC_UNUSED
+static int ga_run_command(const char *argv[], const char *in_str,
+                          const char *action, Error **errp)
+{
+    pid_t pid;
+    int status;
+    int retcode = -1;
+    int infd[2] = { -1, -1 };
+    int outfd[2] = { -1, -1 };
+    char *str = NULL;
+    size_t len = 0;
+
+    if ((in_str && !g_unix_open_pipe(infd, FD_CLOEXEC, NULL)) ||
+        !g_unix_open_pipe(outfd, FD_CLOEXEC, NULL)) {
+        error_setg(errp, "cannot create pipe FDs");
+        goto out;
+    }
+
+    pid = fork();
+    if (pid == 0) {
+        char *cherr = NULL;
+
+        setsid();
+
+        if (in_str) {
+            /* Redirect stdin to infd. */
+            close(infd[1]);
+            dup2(infd[0], 0);
+            close(infd[0]);
+        } else {
+            reopen_fd_to_null(0);
+        }
+
+        /* Redirect stdout/stderr to outfd. */
+        close(outfd[0]);
+        dup2(outfd[1], 1);
+        dup2(outfd[1], 2);
+        close(outfd[1]);
+
+        execvp(argv[0], (char *const *)argv);
+
+        /* Write the cause of failed exec to pipe for the parent to read it. */
+        cherr = g_strdup_printf("failed to exec '%s'", argv[0]);
+        perror(cherr);
+        g_free(cherr);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        goto out;
+    }
+
+    if (in_str) {
+        close(infd[0]);
+        infd[0] = -1;
+        if (qemu_write_full(infd[1], in_str, strlen(in_str)) !=
+                strlen(in_str)) {
+            error_setg_errno(errp, errno, "%s: cannot write to stdin pipe",
+                             action);
+            goto out;
+        }
+        close(infd[1]);
+        infd[1] = -1;
+    }
+
+    ga_pipe_read_str(outfd, &str, &len);
+
+    ga_wait_child(pid, &status, errp);
+    if (*errp) {
+        goto out;
+    }
+
+    if (!WIFEXITED(status)) {
+        if (len) {
+            error_setg(errp, "child process has terminated abnormally: %s",
+                       str);
+        } else {
+            error_setg(errp, "child process has terminated abnormally");
+        }
+        goto out;
+    }
+
+    retcode = WEXITSTATUS(status);
+
+    if (WEXITSTATUS(status)) {
+        if (len) {
+            error_setg(errp, "child process has failed to %s: %s",
+                       action, str);
+        } else {
+            error_setg(errp, "child process has failed to %s: exit status %d",
+                       action, WEXITSTATUS(status));
+        }
+        goto out;
+    }
+
+out:
+    g_free(str);
+
+    if (infd[0] != -1) {
+        close(infd[0]);
+    }
+    if (infd[1] != -1) {
+        close(infd[1]);
+    }
+    if (outfd[0] != -1) {
+        close(outfd[0]);
+    }
+    if (outfd[1] != -1) {
+        close(outfd[1]);
+    }
+
+    return retcode;
+}
+
 void qmp_guest_shutdown(const char *mode, Error **errp)
 {
     const char *shutdown_flag;
-- 
2.39.3

[PATCH v2 3/7] qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper

[Andrey Drobyshev]

Also remove the G_GNUC_UNUSED attribute added in the previous commit from
the helper.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 39 ++++++---------------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 781498418f..003054891f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -103,7 +103,6 @@ static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
  * sending string to stdin and taking error message from
  * stdout/err.
  */
-G_GNUC_UNUSED
 static int ga_run_command(const char *argv[], const char *in_str,
                           const char *action, Error **errp)
 {
@@ -220,8 +219,6 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
 {
     const char *shutdown_flag;
     Error *local_err = NULL;
-    pid_t pid;
-    int status;
 
 #ifdef CONFIG_SOLARIS
     const char *powerdown_flag = "-i5";
@@ -250,46 +247,22 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
         return;
     }
 
-    pid = fork();
-    if (pid == 0) {
-        /* child, start the shutdown */
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
+    const char *argv[] = {"/sbin/shutdown",
 #ifdef CONFIG_SOLARIS
-        execl("/sbin/shutdown", "shutdown", shutdown_flag, "-g0", "-y",
-              "hypervisor initiated shutdown", (char *)NULL);
+                          shutdown_flag, "-g0", "-y",
 #elif defined(CONFIG_BSD)
-        execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
-               "hypervisor initiated shutdown", (char *)NULL);
+                          shutdown_flag, "+0",
 #else
-        execl("/sbin/shutdown", "shutdown", "-h", shutdown_flag, "+0",
-               "hypervisor initiated shutdown", (char *)NULL);
+                          "-h", shutdown_flag, "+0",
 #endif
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+                          "hypervisor initiated shutdown", (char *) NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "shutdown", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        return;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to shutdown");
-        return;
-    }
-
     /* succeeded */
 }
 
-- 
2.39.3

[PATCH v2 4/7] qga/commands-posix: qmp_guest_set_time: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of "/sbin/hwclock", the
exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 003054891f..e44203a273 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -269,21 +269,9 @@ void qmp_guest_shutdown(const char *mode, Error **errp)
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
 {
     int ret;
-    int status;
-    pid_t pid;
     Error *local_err = NULL;
     struct timeval tv;
-    static const char hwclock_path[] = "/sbin/hwclock";
-    static int hwclock_available = -1;
-
-    if (hwclock_available < 0) {
-        hwclock_available = (access(hwclock_path, X_OK) == 0);
-    }
-
-    if (!hwclock_available) {
-        error_setg(errp, QERR_UNSUPPORTED);
-        return;
-    }
+    const char *argv[] = {"/sbin/hwclock", has_time ? "-w" : "-s", NULL};
 
     /* If user has passed a time, validate and set it. */
     if (has_time) {
@@ -314,37 +302,12 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
      * just need to synchronize the hardware clock. However, if no time was
      * passed, user is requesting the opposite: set the system time from the
      * hardware clock (RTC). */
-    pid = fork();
-    if (pid == 0) {
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        /* Use '/sbin/hwclock -w' to set RTC from the system time,
-         * or '/sbin/hwclock -s' to set the system time from RTC. */
-        execl(hwclock_path, "hwclock", has_time ? "-w" : "-s", NULL);
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
-
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "set hardware clock to system time",
+                   &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        return;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "hwclock failed to set hardware clock to system time");
-        return;
-    }
 }
 
 typedef enum {
-- 
2.39.3

[PATCH v2 5/7] qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of the hook executable, as the
exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e44203a273..dd2a7ad2e6 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -726,8 +726,6 @@ static const char *fsfreeze_hook_arg_string[] = {
 
 static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
 {
-    int status;
-    pid_t pid;
     const char *hook;
     const char *arg_str = fsfreeze_hook_arg_string[arg];
     Error *local_err = NULL;
@@ -736,42 +734,15 @@ static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
     if (!hook) {
         return;
     }
-    if (access(hook, X_OK) != 0) {
-        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'", hook);
-        return;
-    }
 
-    slog("executing fsfreeze hook with arg '%s'", arg_str);
-    pid = fork();
-    if (pid == 0) {
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        execl(hook, hook, arg_str, NULL);
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+    const char *argv[] = {hook, arg_str, NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    slog("executing fsfreeze hook with arg '%s'", arg_str);
+    ga_run_command(argv, NULL, "execute fsfreeze hook", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "fsfreeze hook has terminated abnormally");
-        return;
-    }
-
-    status = WEXITSTATUS(status);
-    if (status) {
-        error_setg(errp, "fsfreeze hook has failed with status %d", status);
-        return;
-    }
 }
 
 /*
-- 
2.39.3

[PATCH v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs

[Andrey Drobyshev]

We replace the direct call to open() with a "sh -c 'echo ...'" call, so
that it becomes an executable command.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index dd2a7ad2e6..f3f4a05e2d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
     Error *local_err = NULL;
     const char *sysfile_strs[3] = {"disk", "mem", NULL};
     const char *sysfile_str = sysfile_strs[mode];
-    pid_t pid;
-    int status;
 
     if (!sysfile_str) {
         error_setg(errp, "unknown guest suspend mode");
         return;
     }
 
-    pid = fork();
-    if (!pid) {
-        /* child */
-        int fd;
-
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
-        if (fd < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        _exit(EXIT_SUCCESS);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
+    g_autofree char *echo_cmd = g_strdup_printf(
+        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
+    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "suspend", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to suspend");
-    }
-
 }
 
 static void guest_suspend(SuspendMode mode, Error **errp)
-- 
2.39.3

[PATCH v2 7/7] qga/commands-posix: qmp_guest_set_user_password: use ga_run_command helper

[Andrey Drobyshev]

There's no need to check for the existence of the "chpasswd", "pw"
executables, as the exec() call will do that for us.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 96 ++++++--------------------------------------
 1 file changed, 13 insertions(+), 83 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index f3f4a05e2d..f2e9496b80 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2144,14 +2144,8 @@ void qmp_guest_set_user_password(const char *username,
                                  Error **errp)
 {
     Error *local_err = NULL;
-    char *passwd_path = NULL;
-    pid_t pid;
-    int status;
-    int datafd[2] = { -1, -1 };
-    char *rawpasswddata = NULL;
+    g_autofree char *rawpasswddata = NULL;
     size_t rawpasswdlen;
-    char *chpasswddata = NULL;
-    size_t chpasswdlen;
 
     rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
     if (!rawpasswddata) {
@@ -2162,95 +2156,31 @@ void qmp_guest_set_user_password(const char *username,
 
     if (strchr(rawpasswddata, '\n')) {
         error_setg(errp, "forbidden characters in raw password");
-        goto out;
+        return;
     }
 
     if (strchr(username, '\n') ||
         strchr(username, ':')) {
         error_setg(errp, "forbidden characters in username");
-        goto out;
+        return;
     }
 
 #ifdef __FreeBSD__
-    chpasswddata = g_strdup(rawpasswddata);
-    passwd_path = g_find_program_in_path("pw");
+    g_autofree char *chpasswdata = g_strdup(rawpasswddata);
+    const char *crypt_flag = (crypted) ? "-H" : "-h";
+    const char *argv[] = {"pw", "usermod", "-n", username,
+                          crypt_flag, "0", NULL};
 #else
-    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
-    passwd_path = g_find_program_in_path("chpasswd");
+    g_autofree char *chpasswddata = g_strdup_printf("%s:%s\n", username,
+                                                    rawpasswddata);
+    const char *crypt_flag = (crypted) ? "-e" : NULL;
+    const char *argv[] = {"chpasswd", crypt_flag, NULL};
 #endif
 
-    chpasswdlen = strlen(chpasswddata);
-
-    if (!passwd_path) {
-        error_setg(errp, "cannot find 'passwd' program in PATH");
-        goto out;
-    }
-
-    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
-        error_setg(errp, "cannot create pipe FDs");
-        goto out;
-    }
-
-    pid = fork();
-    if (pid == 0) {
-        close(datafd[1]);
-        /* child */
-        setsid();
-        dup2(datafd[0], 0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-#ifdef __FreeBSD__
-        const char *h_arg;
-        h_arg = (crypted) ? "-H" : "-h";
-        execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0", NULL);
-#else
-        if (crypted) {
-            execl(passwd_path, "chpasswd", "-e", NULL);
-        } else {
-            execl(passwd_path, "chpasswd", NULL);
-        }
-#endif
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
-    }
-    close(datafd[0]);
-    datafd[0] = -1;
-
-    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
-        error_setg_errno(errp, errno, "cannot write new account password");
-        goto out;
-    }
-    close(datafd[1]);
-    datafd[1] = -1;
-
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, chpasswddata, "set user password", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        goto out;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to set user password");
-        goto out;
-    }
-
-out:
-    g_free(chpasswddata);
-    g_free(rawpasswddata);
-    g_free(passwd_path);
-    if (datafd[0] != -1) {
-        close(datafd[0]);
-    }
-    if (datafd[1] != -1) {
-        close(datafd[1]);
+        return;
     }
 }
 #else /* __linux__ || __FreeBSD__ */
-- 
2.39.3

Re: [PATCH v2 0/7] qga/commands-posix: replace code duplicating commands with a helper

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

For series
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Fri, Mar 1, 2024 at 7:29 PM Andrey Drobyshev <
andrey.drobyshev@virtuozzo.com> wrote:

> v1 --> v2:
>  * Replace commit for guest-get-fsinfo: instead of reporting statvfs(3)
>    values directly, keep the old ones but add an additional optional
>    field 'total-bytes-root', only reported in POSIX.  Also tweak the
>    fields description in qga/qapi-schema.json to document where the
>    values come from.
>
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg05766.html
>
> Andrey Drobyshev (7):
>   qga: guest-get-fsinfo: add optional 'total-bytes-root' field
>   qga: introduce ga_run_command() helper for guest cmd execution
>   qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper
>   qga/commands-posix: qmp_guest_set_time: use ga_run_command helper
>   qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper
>   qga/commands-posix: use ga_run_command helper when suspending via
>     sysfs
>   qga/commands-posix: qmp_guest_set_user_password: use ga_run_command
>     helper
>
>  qga/commands-posix.c | 389 +++++++++++++++++++------------------------
>  qga/commands-win32.c |   1 +
>  qga/qapi-schema.json |  12 +-
>  3 files changed, 182 insertions(+), 220 deletions(-)
>
> --
> 2.39.3
>
>

[-- Attachment #2: Type: text/html, Size: 1966 bytes --]

Re: [PATCH v2 0/7] qga/commands-posix: replace code duplicating commands with a helper

[Dehan Meng]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

QE tested this series's patches. qga-related commands worked successfully.

Tested-by: Dehan Meng <demeng@redhat.com>


On Mon, Mar 4, 2024 at 8:00 PM Konstantin Kostiuk <kkostiuk@redhat.com>
wrote:

> For series
> Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
>
> On Fri, Mar 1, 2024 at 7:29 PM Andrey Drobyshev <
> andrey.drobyshev@virtuozzo.com> wrote:
>
>> v1 --> v2:
>>  * Replace commit for guest-get-fsinfo: instead of reporting statvfs(3)
>>    values directly, keep the old ones but add an additional optional
>>    field 'total-bytes-root', only reported in POSIX.  Also tweak the
>>    fields description in qga/qapi-schema.json to document where the
>>    values come from.
>>
>> v1:
>> https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg05766.html
>>
>> Andrey Drobyshev (7):
>>   qga: guest-get-fsinfo: add optional 'total-bytes-root' field
>>   qga: introduce ga_run_command() helper for guest cmd execution
>>   qga/commands-posix: qmp_guest_shutdown: use ga_run_command helper
>>   qga/commands-posix: qmp_guest_set_time: use ga_run_command helper
>>   qga/commands-posix: execute_fsfreeze_hook: use ga_run_command helper
>>   qga/commands-posix: use ga_run_command helper when suspending via
>>     sysfs
>>   qga/commands-posix: qmp_guest_set_user_password: use ga_run_command
>>     helper
>>
>>  qga/commands-posix.c | 389 +++++++++++++++++++------------------------
>>  qga/commands-win32.c |   1 +
>>  qga/qapi-schema.json |  12 +-
>>  3 files changed, 182 insertions(+), 220 deletions(-)
>>
>> --
>> 2.39.3
>>
>>

[-- Attachment #2: Type: text/html, Size: 3559 bytes --]

Re: [PATCH v2 2/7] qga: introduce ga_run_command() helper for guest cmd execution

[Daniel P. Berrangé]

On Fri, Mar 01, 2024 at 07:28:53PM +0200, Andrey Drobyshev wrote:
> When executing guest commands in *nix environment, we repeat the same
> fork/exec pattern multiple times.  Let's just separate it into a single
> helper which would also be able to feed input data into the launched
> process' stdin.  This way we can avoid code duplication.
> 
> To keep the history more bisectable, let's replace qmp commands
> implementations one by one.  Also add G_GNUC_UNUSED attribute to the
> helper and remove it in the next commit.
> 
> Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 140 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8207c4c47e..781498418f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -76,6 +76,146 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
>      g_assert(rpid == pid);
>  }
>  
> +static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
> +{
> +    ssize_t n;
> +    char buf[1024];
> +    close(fd[1]);
> +    fd[1] = -1;
> +    while ((n = read(fd[0], buf, sizeof(buf))) != 0) {
> +        if (n < 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else {
> +                break;

This is a fatal error condition....

> +            }
> +        }
> +        *str = g_realloc(*str, *len + n);
> +        memcpy(*str + *len, buf, n);
> +        *len += n;
> +    }
> +    close(fd[0]);
> +    fd[0] = -1;

....and yet as far as the caller is concerned we're not indicating
any sense of failure here. They're just being returned a partially
read data stream as if it were successful. IMHO this needs to be
reported properly.


Nothing in this method has NUL terminated 'str', so we are
relying on the caller *always* honouring 'len', but.....

> +}
> +
> +/*
> + * Helper to run command with input/output redirection,
> + * sending string to stdin and taking error message from
> + * stdout/err.
> + */
> +G_GNUC_UNUSED
> +static int ga_run_command(const char *argv[], const char *in_str,
> +                          const char *action, Error **errp)
> +{
> +    pid_t pid;
> +    int status;
> +    int retcode = -1;
> +    int infd[2] = { -1, -1 };
> +    int outfd[2] = { -1, -1 };
> +    char *str = NULL;
> +    size_t len = 0;
> +
> +    if ((in_str && !g_unix_open_pipe(infd, FD_CLOEXEC, NULL)) ||
> +        !g_unix_open_pipe(outfd, FD_CLOEXEC, NULL)) {
> +        error_setg(errp, "cannot create pipe FDs");
> +        goto out;
> +    }
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        char *cherr = NULL;
> +
> +        setsid();
> +
> +        if (in_str) {
> +            /* Redirect stdin to infd. */
> +            close(infd[1]);
> +            dup2(infd[0], 0);
> +            close(infd[0]);
> +        } else {
> +            reopen_fd_to_null(0);
> +        }
> +
> +        /* Redirect stdout/stderr to outfd. */
> +        close(outfd[0]);
> +        dup2(outfd[1], 1);
> +        dup2(outfd[1], 2);
> +        close(outfd[1]);
> +
> +        execvp(argv[0], (char *const *)argv);
> +
> +        /* Write the cause of failed exec to pipe for the parent to read it. */
> +        cherr = g_strdup_printf("failed to exec '%s'", argv[0]);
> +        perror(cherr);
> +        g_free(cherr);
> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        error_setg_errno(errp, errno, "failed to create child process");
> +        goto out;
> +    }
> +
> +    if (in_str) {
> +        close(infd[0]);
> +        infd[0] = -1;
> +        if (qemu_write_full(infd[1], in_str, strlen(in_str)) !=
> +                strlen(in_str)) {
> +            error_setg_errno(errp, errno, "%s: cannot write to stdin pipe",
> +                             action);
> +            goto out;
> +        }
> +        close(infd[1]);
> +        infd[1] = -1;
> +    }
> +
> +    ga_pipe_read_str(outfd, &str, &len);
> +
> +    ga_wait_child(pid, &status, errp);
> +    if (*errp) {
> +        goto out;
> +    }
> +
> +    if (!WIFEXITED(status)) {
> +        if (len) {
> +            error_setg(errp, "child process has terminated abnormally: %s",
> +                       str);

...this is reading 'str' without honouring 'len', so is likely
an array out of bounds read.

> +        } else {
> +            error_setg(errp, "child process has terminated abnormally");
> +        }
> +        goto out;
> +    }
> +
> +    retcode = WEXITSTATUS(status);
> +
> +    if (WEXITSTATUS(status)) {
> +        if (len) {
> +            error_setg(errp, "child process has failed to %s: %s",
> +                       action, str);

Again, array out of bounds is likely

> +        } else {
> +            error_setg(errp, "child process has failed to %s: exit status %d",
> +                       action, WEXITSTATUS(status));
> +        }
> +        goto out;
> +    }
> +
> +out:
> +    g_free(str);
> +
> +    if (infd[0] != -1) {
> +        close(infd[0]);
> +    }
> +    if (infd[1] != -1) {
> +        close(infd[1]);
> +    }
> +    if (outfd[0] != -1) {
> +        close(outfd[0]);
> +    }
> +    if (outfd[1] != -1) {
> +        close(outfd[1]);
> +    }
> +
> +    return retcode;
> +}
> +
>  void qmp_guest_shutdown(const char *mode, Error **errp)
>  {
>      const char *shutdown_flag;
> -- 
> 2.39.3
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs

[Daniel P. Berrangé]

On Fri, Mar 01, 2024 at 07:28:57PM +0200, Andrey Drobyshev wrote:
> We replace the direct call to open() with a "sh -c 'echo ...'" call, so
> that it becomes an executable command.

Introduced an indirection via the shell is a significant step
backwards IMHO.

> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 36 ++++--------------------------------
>  1 file changed, 4 insertions(+), 32 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dd2a7ad2e6..f3f4a05e2d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>      Error *local_err = NULL;
>      const char *sysfile_strs[3] = {"disk", "mem", NULL};
>      const char *sysfile_str = sysfile_strs[mode];
> -    pid_t pid;
> -    int status;
>  
>      if (!sysfile_str) {
>          error_setg(errp, "unknown guest suspend mode");
>          return;
>      }
>  
> -    pid = fork();
> -    if (!pid) {
> -        /* child */
> -        int fd;
> -
> -        setsid();
> -        reopen_fd_to_null(0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> -        if (fd < 0) {
> -            _exit(EXIT_FAILURE);
> -        }
> -
> -        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
> -            _exit(EXIT_FAILURE);
> -        }
> -
> -        _exit(EXIT_SUCCESS);


This pre-existing code is strange to me.  Why do we need to fork a
new process in order to write to /sys/power/state ?

Looking at the original commit

  commit 11d0f1255bd5651f628280dc96c4ce9d63ae9236
  Author: Luiz Capitulino <lcapitulino@redhat.com>
  Date:   Tue Feb 28 11:03:03 2012 -0300

    qemu-ga: add guest-suspend-disk
    

The code made a little more sense, as after fork() it first
tried to execve  'pm-utils', and then had the sysfs codepath
as a fallback. IOW having the sysfs code after fork() was a
much easier code structure.

This was all changed in

  commit 246d76eba1944d7e59affb288ec27d7fcfb5d256
  Author: Daniel Henrique Barboza <danielhb413@gmail.com>
  Date:   Thu Jun 21 07:21:50 2018 -0300

    qga: guest_suspend: decoupling pm-utils and sys logic


so the pm-utils logic runs in a separate forked child
from the sysfs logic.

AFAICT, that should have made it completely redundant to
fork a process to access /sys/power/state.

Does anyone know of a reason to keep the fork() here ? Of
not we should just be calling 'g_file_set_contents' without
fork

> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        return;
> -    }
> +    g_autofree char *echo_cmd = g_strdup_printf(
> +        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
> +    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
>  
> -    ga_wait_child(pid, &status, &local_err);
> +    ga_run_command(argv, NULL, "suspend", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp, "child process has failed to suspend");
> -    }
> -
>  }
>  
>  static void guest_suspend(SuspendMode mode, Error **errp)
> -- 
> 2.39.3
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 7/7] qga/commands-posix: qmp_guest_set_user_password: use ga_run_command helper

[Daniel P. Berrangé]

On Fri, Mar 01, 2024 at 07:28:58PM +0200, Andrey Drobyshev wrote:
> There's no need to check for the existence of the "chpasswd", "pw"
> executables, as the exec() call will do that for us.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 96 ++++++--------------------------------------
>  1 file changed, 13 insertions(+), 83 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f3f4a05e2d..f2e9496b80 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2144,14 +2144,8 @@ void qmp_guest_set_user_password(const char *username,
>                                   Error **errp)
>  {
>      Error *local_err = NULL;
> -    char *passwd_path = NULL;
> -    pid_t pid;
> -    int status;
> -    int datafd[2] = { -1, -1 };
> -    char *rawpasswddata = NULL;
> +    g_autofree char *rawpasswddata = NULL;
>      size_t rawpasswdlen;
> -    char *chpasswddata = NULL;
> -    size_t chpasswdlen;
>  
>      rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
>      if (!rawpasswddata) {
> @@ -2162,95 +2156,31 @@ void qmp_guest_set_user_password(const char *username,
>  
>      if (strchr(rawpasswddata, '\n')) {
>          error_setg(errp, "forbidden characters in raw password");
> -        goto out;
> +        return;
>      }
>  
>      if (strchr(username, '\n') ||
>          strchr(username, ':')) {
>          error_setg(errp, "forbidden characters in username");
> -        goto out;
> +        return;
>      }
>  
>  #ifdef __FreeBSD__
> -    chpasswddata = g_strdup(rawpasswddata);
> -    passwd_path = g_find_program_in_path("pw");
> +    g_autofree char *chpasswdata = g_strdup(rawpasswddata);
> +    const char *crypt_flag = (crypted) ? "-H" : "-h";
> +    const char *argv[] = {"pw", "usermod", "-n", username,
> +                          crypt_flag, "0", NULL};
>  #else
> -    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> -    passwd_path = g_find_program_in_path("chpasswd");
> +    g_autofree char *chpasswddata = g_strdup_printf("%s:%s\n", username,
> +                                                    rawpasswddata);
> +    const char *crypt_flag = (crypted) ? "-e" : NULL;

Style nit-pick - no '(...)' around 'crypted' is needed here, or
the other place later in this method.

Yes, that was a pre-existing issue, but since you're refactoring
the code, might as well kill the redundant brackets.

> +    const char *argv[] = {"chpasswd", crypt_flag, NULL};
>  #endif
>  
> -    chpasswdlen = strlen(chpasswddata);
> -
> -    if (!passwd_path) {
> -        error_setg(errp, "cannot find 'passwd' program in PATH");
> -        goto out;
> -    }
> -
> -    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
> -        error_setg(errp, "cannot create pipe FDs");
> -        goto out;
> -    }
> -
> -    pid = fork();
> -    if (pid == 0) {
> -        close(datafd[1]);
> -        /* child */
> -        setsid();
> -        dup2(datafd[0], 0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -#ifdef __FreeBSD__
> -        const char *h_arg;
> -        h_arg = (crypted) ? "-H" : "-h";
> -        execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0", NULL);
> -#else
> -        if (crypted) {
> -            execl(passwd_path, "chpasswd", "-e", NULL);
> -        } else {
> -            execl(passwd_path, "chpasswd", NULL);
> -        }
> -#endif
> -        _exit(EXIT_FAILURE);
> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        goto out;
> -    }
> -    close(datafd[0]);
> -    datafd[0] = -1;
> -
> -    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
> -        error_setg_errno(errp, errno, "cannot write new account password");
> -        goto out;
> -    }
> -    close(datafd[1]);
> -    datafd[1] = -1;
> -
> -    ga_wait_child(pid, &status, &local_err);
> +    ga_run_command(argv, chpasswddata, "set user password", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        goto out;
> -    }
> -
> -    if (!WIFEXITED(status)) {
> -        error_setg(errp, "child process has terminated abnormally");
> -        goto out;
> -    }
> -
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp, "child process has failed to set user password");
> -        goto out;
> -    }
> -
> -out:
> -    g_free(chpasswddata);
> -    g_free(rawpasswddata);
> -    g_free(passwd_path);
> -    if (datafd[0] != -1) {
> -        close(datafd[0]);
> -    }
> -    if (datafd[1] != -1) {
> -        close(datafd[1]);
> +        return;
>      }
>  }
>  #else /* __linux__ || __FreeBSD__ */
> -- 
> 2.39.3
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 7/7] qga/commands-posix: qmp_guest_set_user_password: use ga_run_command helper

[Andrey Drobyshev]

On 3/5/24 20:38, Daniel P. Berrangé wrote:
> On Fri, Mar 01, 2024 at 07:28:58PM +0200, Andrey Drobyshev wrote:
>> There's no need to check for the existence of the "chpasswd", "pw"
>> executables, as the exec() call will do that for us.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 96 ++++++--------------------------------------
>>  1 file changed, 13 insertions(+), 83 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index f3f4a05e2d..f2e9496b80 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2144,14 +2144,8 @@ void qmp_guest_set_user_password(const char *username,
>>                                   Error **errp)
>>  {
>>      Error *local_err = NULL;
>> -    char *passwd_path = NULL;
>> -    pid_t pid;
>> -    int status;
>> -    int datafd[2] = { -1, -1 };
>> -    char *rawpasswddata = NULL;
>> +    g_autofree char *rawpasswddata = NULL;
>>      size_t rawpasswdlen;
>> -    char *chpasswddata = NULL;
>> -    size_t chpasswdlen;
>>
>>      rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
>>      if (!rawpasswddata) {
>> @@ -2162,95 +2156,31 @@ void qmp_guest_set_user_password(const char *username,
>>
>>      if (strchr(rawpasswddata, '\n')) {
>>          error_setg(errp, "forbidden characters in raw password");
>> -        goto out;
>> +        return;
>>      }
>>
>>      if (strchr(username, '\n') ||
>>          strchr(username, ':')) {
>>          error_setg(errp, "forbidden characters in username");
>> -        goto out;
>> +        return;
>>      }
>>
>>  #ifdef __FreeBSD__
>> -    chpasswddata = g_strdup(rawpasswddata);
>> -    passwd_path = g_find_program_in_path("pw");
>> +    g_autofree char *chpasswdata = g_strdup(rawpasswddata);
>> +    const char *crypt_flag = (crypted) ? "-H" : "-h";
>> +    const char *argv[] = {"pw", "usermod", "-n", username,
>> +                          crypt_flag, "0", NULL};
>>  #else
>> -    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
>> -    passwd_path = g_find_program_in_path("chpasswd");
>> +    g_autofree char *chpasswddata = g_strdup_printf("%s:%s\n", username,
>> +                                                    rawpasswddata);
>> +    const char *crypt_flag = (crypted) ? "-e" : NULL;
> 
> Style nit-pick - no '(...)' around 'crypted' is needed here, or
> the other place later in this method.
> 
> Yes, that was a pre-existing issue, but since you're refactoring
> the code, might as well kill the redundant brackets.
> 
> [...]

Sure, let's get rid of them. Thanks.

Re: [PATCH v2 2/7] qga: introduce ga_run_command() helper for guest cmd execution

[Andrey Drobyshev]

On 3/5/24 19:58, Daniel P. Berrangé wrote:
> On Fri, Mar 01, 2024 at 07:28:53PM +0200, Andrey Drobyshev wrote:
>> When executing guest commands in *nix environment, we repeat the same
>> fork/exec pattern multiple times.  Let's just separate it into a single
>> helper which would also be able to feed input data into the launched
>> process' stdin.  This way we can avoid code duplication.
>>
>> To keep the history more bisectable, let's replace qmp commands
>> implementations one by one.  Also add G_GNUC_UNUSED attribute to the
>> helper and remove it in the next commit.
>>
>> Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 140 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 140 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 8207c4c47e..781498418f 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -76,6 +76,146 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
>>      g_assert(rpid == pid);
>>  }
>>
>> +static void ga_pipe_read_str(int fd[2], char **str, size_t *len)
>> +{
>> +    ssize_t n;
>> +    char buf[1024];
>> +    close(fd[1]);
>> +    fd[1] = -1;
>> +    while ((n = read(fd[0], buf, sizeof(buf))) != 0) {
>> +        if (n < 0) {
>> +            if (errno == EINTR) {
>> +                continue;
>> +            } else {
>> +                break;
> 
> This is a fatal error condition....
> 
>> +            }
>> +        }
>> +        *str = g_realloc(*str, *len + n);
>> +        memcpy(*str + *len, buf, n);
>> +        *len += n;
>> +    }
>> +    close(fd[0]);
>> +    fd[0] = -1;
> 
> ....and yet as far as the caller is concerned we're not indicating
> any sense of failure here. They're just being returned a partially
> read data stream as if it were successful. IMHO this needs to be
> reported properly.
>

Agreed.  We might make this helper return -errno in case of an erroneous
read from pipe, check the value in the caller and do error_setg_errno()
if smth went wrong.

> 
> Nothing in this method has NUL terminated 'str', so we are
> relying on the caller *always* honouring 'len', but.....
>

Agreed as well.  Let's allocate +1 additional byte and store '\0' in
there on every iteration, making sure our result is always
null-terminated. That way we won't need to check 'len' in the caller.

>> +}
>> +
>> +/*
>> + * Helper to run command with input/output redirection,
>> + * sending string to stdin and taking error message from
>> + * stdout/err.
>> + */
>> +G_GNUC_UNUSED
>> +static int ga_run_command(const char *argv[], const char *in_str,
>> +                          const char *action, Error **errp)
>> +{
>> +    pid_t pid;
>> +    int status;
>> +    int retcode = -1;
>> +    int infd[2] = { -1, -1 };
>> +    int outfd[2] = { -1, -1 };
>> +    char *str = NULL;
>> +    size_t len = 0;
>> +
>> +    if ((in_str && !g_unix_open_pipe(infd, FD_CLOEXEC, NULL)) ||
>> +        !g_unix_open_pipe(outfd, FD_CLOEXEC, NULL)) {
>> +        error_setg(errp, "cannot create pipe FDs");
>> +        goto out;
>> +    }
>> +
>> +    pid = fork();
>> +    if (pid == 0) {
>> +        char *cherr = NULL;
>> +
>> +        setsid();
>> +
>> +        if (in_str) {
>> +            /* Redirect stdin to infd. */
>> +            close(infd[1]);
>> +            dup2(infd[0], 0);
>> +            close(infd[0]);
>> +        } else {
>> +            reopen_fd_to_null(0);
>> +        }
>> +
>> +        /* Redirect stdout/stderr to outfd. */
>> +        close(outfd[0]);
>> +        dup2(outfd[1], 1);
>> +        dup2(outfd[1], 2);
>> +        close(outfd[1]);
>> +
>> +        execvp(argv[0], (char *const *)argv);
>> +
>> +        /* Write the cause of failed exec to pipe for the parent to read it. */
>> +        cherr = g_strdup_printf("failed to exec '%s'", argv[0]);
>> +        perror(cherr);
>> +        g_free(cherr);
>> +        _exit(EXIT_FAILURE);
>> +    } else if (pid < 0) {
>> +        error_setg_errno(errp, errno, "failed to create child process");
>> +        goto out;
>> +    }
>> +
>> +    if (in_str) {
>> +        close(infd[0]);
>> +        infd[0] = -1;
>> +        if (qemu_write_full(infd[1], in_str, strlen(in_str)) !=
>> +                strlen(in_str)) {
>> +            error_setg_errno(errp, errno, "%s: cannot write to stdin pipe",
>> +                             action);
>> +            goto out;
>> +        }
>> +        close(infd[1]);
>> +        infd[1] = -1;
>> +    }
>> +
>> +    ga_pipe_read_str(outfd, &str, &len);
>> +
>> +    ga_wait_child(pid, &status, errp);
>> +    if (*errp) {
>> +        goto out;
>> +    }
>> +
>> +    if (!WIFEXITED(status)) {
>> +        if (len) {
>> +            error_setg(errp, "child process has terminated abnormally: %s",
>> +                       str);
> 
> ...this is reading 'str' without honouring 'len', so is likely
> an array out of bounds read.
> 
>> +        } else {
>> +            error_setg(errp, "child process has terminated abnormally");
>> +        }
>> +        goto out;
>> +    }
>> +
>> +    retcode = WEXITSTATUS(status);
>> +
>> +    if (WEXITSTATUS(status)) {
>> +        if (len) {
>> +            error_setg(errp, "child process has failed to %s: %s",
>> +                       action, str);
> 
> Again, array out of bounds is likely
> 
>> +        } else {
>> +            error_setg(errp, "child process has failed to %s: exit status %d",
>> +                       action, WEXITSTATUS(status));
>> +        }
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    g_free(str);
>> +
>> +    if (infd[0] != -1) {
>> +        close(infd[0]);
>> +    }
>> +    if (infd[1] != -1) {
>> +        close(infd[1]);
>> +    }
>> +    if (outfd[0] != -1) {
>> +        close(outfd[0]);
>> +    }
>> +    if (outfd[1] != -1) {
>> +        close(outfd[1]);
>> +    }
>> +
>> +    return retcode;
>> +}
>> +
>>  void qmp_guest_shutdown(const char *mode, Error **errp)
>>  {
>>      const char *shutdown_flag;
>> --
>> 2.39.3
>>
>>
> 
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs

[Andrey Drobyshev]

On 3/5/24 20:34, Daniel P. Berrangé wrote:
> [Вы нечасто получаете письма от berrange@redhat.com. Узнайте, почему это важно, по адресу https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Fri, Mar 01, 2024 at 07:28:57PM +0200, Andrey Drobyshev wrote:
>> We replace the direct call to open() with a "sh -c 'echo ...'" call, so
>> that it becomes an executable command.
> 
> Introduced an indirection via the shell is a significant step
> backwards IMHO.
> 
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 36 ++++--------------------------------
>>  1 file changed, 4 insertions(+), 32 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index dd2a7ad2e6..f3f4a05e2d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>>      Error *local_err = NULL;
>>      const char *sysfile_strs[3] = {"disk", "mem", NULL};
>>      const char *sysfile_str = sysfile_strs[mode];
>> -    pid_t pid;
>> -    int status;
>>
>>      if (!sysfile_str) {
>>          error_setg(errp, "unknown guest suspend mode");
>>          return;
>>      }
>>
>> -    pid = fork();
>> -    if (!pid) {
>> -        /* child */
>> -        int fd;
>> -
>> -        setsid();
>> -        reopen_fd_to_null(0);
>> -        reopen_fd_to_null(1);
>> -        reopen_fd_to_null(2);
>> -
>> -        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>> -        if (fd < 0) {
>> -            _exit(EXIT_FAILURE);
>> -        }
>> -
>> -        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
>> -            _exit(EXIT_FAILURE);
>> -        }
>> -
>> -        _exit(EXIT_SUCCESS);
> 
> 
> This pre-existing code is strange to me.  Why do we need to fork a
> new process in order to write to /sys/power/state ?
> 
> Looking at the original commit
> 
>   commit 11d0f1255bd5651f628280dc96c4ce9d63ae9236
>   Author: Luiz Capitulino <lcapitulino@redhat.com>
>   Date:   Tue Feb 28 11:03:03 2012 -0300
> 
>     qemu-ga: add guest-suspend-disk
> 
> 
> The code made a little more sense, as after fork() it first
> tried to execve  'pm-utils', and then had the sysfs codepath
> as a fallback. IOW having the sysfs code after fork() was a
> much easier code structure.
> 
> This was all changed in
> 
>   commit 246d76eba1944d7e59affb288ec27d7fcfb5d256
>   Author: Daniel Henrique Barboza <danielhb413@gmail.com>
>   Date:   Thu Jun 21 07:21:50 2018 -0300
> 
>     qga: guest_suspend: decoupling pm-utils and sys logic
> 
> 
> so the pm-utils logic runs in a separate forked child
> from the sysfs logic.
> 
> AFAICT, that should have made it completely redundant to
> fork a process to access /sys/power/state.
> 
> Does anyone know of a reason to keep the fork() here ? Of
> not we should just be calling 'g_file_set_contents' without
> fork
> 

In the original commit message Luiz Capitulino explained that multiple
forks are needed to properly reap child processes needed for execl().
AFAIU since writing to /sys/power/state doesn't require execl(), fork()
isn't needed either.  I think the 2nd commit you mention simply kept
things as they were since it wasn't the original goal of the patch.  So
we're just looking at legacy code.

I agree that in this case my original patch is redundant and we may
replace it with smth like g_file_set_contents().  I'll add it in v3.

>> -    } else if (pid < 0) {
>> -        error_setg_errno(errp, errno, "failed to create child process");
>> -        return;
>> -    }
>> +    g_autofree char *echo_cmd = g_strdup_printf(
>> +        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
>> +    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
>>
>> -    ga_wait_child(pid, &status, &local_err);
>> +    ga_run_command(argv, NULL, "suspend", &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>> -
>> -    if (WEXITSTATUS(status)) {
>> -        error_setg(errp, "child process has failed to suspend");
>> -    }
>> -
>>  }
>>
>>  static void guest_suspend(SuspendMode mode, Error **errp)
>> --
>> 2.39.3
>>
>>
> 
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>