diff mbox

[1/2] create ga_run_program() helper for guest-set-user-password

Message ID 1452081699-11894-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Jan. 6, 2016, 12:01 p.m. UTC
From: Yuriy Pudgorodskiy <yur@virtuozzo.com>

This helper properly starts chpasswd and collects stdout/stderr of this
program to report it as error to the caller.

The code will be reused later to run useradd in addition to chpasswd.

This code is made specifically for Linux and is inside ifdef Linux braces.

Signed-off-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 201 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 141 insertions(+), 60 deletions(-)

Comments

Denis V. Lunev Jan. 6, 2016, 1:50 p.m. UTC | #1
On 01/06/2016 03:01 PM, Denis V. Lunev wrote:
> From: Yuriy Pudgorodskiy <yur@virtuozzo.com>
>
> This helper properly starts chpasswd and collects stdout/stderr of this
> program to report it as error to the caller.
>
> The code will be reused later to run useradd in addition to chpasswd.
>
> This code is made specifically for Linux and is inside ifdef Linux braces.
>
> Signed-off-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>   qga/commands-posix.c | 201 ++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 141 insertions(+), 60 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8fe708f..53e8d3b 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -75,6 +75,28 @@ 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;
> +}
> +
>   void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
>   {
>       const char *shutdown_flag;
> @@ -1952,20 +1974,128 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>       return processed;
>   }
>   
> +/* Helper to run command with input/output redirection,
> + * sending string to stdin and taking error message from
> + * stdout/err
> + */
> +static void ga_run_program(const char *argv[],
> +                           const char *in_str,
> +                           const char *action, Error **errp)
> +{
> +    pid_t pid;
> +    int status;
> +    int infd[2] = { -1, -1 };
> +    int outfd[2] = { -1, -1 };
> +    char *str = NULL;
> +    size_t len = 0;
> +
> +    if (in_str) {
> +        if (pipe(infd) < 0) {
> +            error_setg(errp, "cannot create pipe FDs");
> +            goto out;
> +        }
> +    }
> +
> +    if (pipe(outfd) < 0) {
> +        error_setg(errp, "cannot create pipe FDs");
> +        goto out;
> +    }
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        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]);
> +
> +        execve(argv[0], (char *const *)argv, environ);
> +        _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;
> +    }
> +
> +    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]);
> +    }
> +}
> +
>   void qmp_guest_set_user_password(const char *username,
>                                    const char *password,
>                                    bool crypted,
>                                    Error **errp)
>   {
> -    Error *local_err = NULL;
>       char *passwd_path = NULL;
> -    pid_t pid;
> -    int status;
> -    int datafd[2] = { -1, -1 };
>       char *rawpasswddata = NULL;
>       size_t rawpasswdlen;
>       char *chpasswddata = NULL;
> -    size_t chpasswdlen;
> +    const char *chpasswd_argv[] = { NULL /*path*/, "chpasswd", "-e", NULL };
>   
>       rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
>       if (!rawpasswddata) {
> @@ -1986,75 +2116,26 @@ void qmp_guest_set_user_password(const char *username,
>       }
>   
>       chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> -    chpasswdlen = strlen(chpasswddata);
>   
>       passwd_path = g_find_program_in_path("chpasswd");
> -
>       if (!passwd_path) {
>           error_setg(errp, "cannot find 'passwd' program in PATH");
>           goto out;
>       }
>   
> -    if (pipe(datafd) < 0) {
> -        error_setg(errp, "cannot create pipe FDs");
> -        goto out;
> -    }
> +    chpasswd_argv[0] = passwd_path;
>   
> -    pid = fork();
> -    if (pid == 0) {
> -        close(datafd[1]);
> -        /* child */
> -        setsid();
> -        dup2(datafd[0], 0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -        if (crypted) {
> -            execle(passwd_path, "chpasswd", "-e", NULL, environ);
> -        } else {
> -            execle(passwd_path, "chpasswd", NULL, environ);
> -        }
> -        _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);
> -    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;
> +    /* set password for existed user */
> +    if (!crypted) {
> +        /* wipe -e option */
> +        chpasswd_argv[2] = NULL;
>       }
> +    ga_run_program(chpasswd_argv, chpasswddata, "set user password", errp);
>   
>   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]);
> -    }
>   }
>   
>   static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
hmmm....

Yur,

it seems that you have re-invented the wheel with

gboolean 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gboolean>
g_spawn_sync (/|const gchar 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
*working_directory|/,
               /|gchar 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
**argv|/,
               /|gchar 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
**envp|/,
               /|GSpawnFlags 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnFlags> 
flags|/,
               /|GSpawnChildSetupFunc 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnChildSetupFunc> 
child_setup|/,
               /|gpointer 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gpointer> 
user_data|/,
               /|gchar 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
**standard_output|/,
               /|gchar 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
**standard_error|/,
               /|gint 
<https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gint> 
*exit_status|/,
               /|GError 
<https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#GError> 
**error|/);


Executes a child synchronously (waits for the child to exit before 
returning). All output from the child is stored 
in/|standard_output|/and/|standard_error|/, if those parameters are 
non-|NULL| 
<https://developer.gnome.org/glib/stable/glib-Standard-Macros.html#NULL:CAPS>. 
Note that you must set the|G_SPAWN_STDOUT_TO_DEV_NULL| 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#G-SPAWN-STDOUT-TO-DEV-NULL:CAPS>and|G_SPAWN_STDERR_TO_DEV_NULL| 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#G-SPAWN-STDERR-TO-DEV-NULL:CAPS>flags 
when passing|NULL| 
<https://developer.gnome.org/glib/stable/glib-Standard-Macros.html#NULL:CAPS>for/|standard_output|/and/|standard_error|/.

If/|exit_status|/is non-|NULL| 
<https://developer.gnome.org/glib/stable/glib-Standard-Macros.html#NULL:CAPS>, 
the platform-specific exit status of the child is stored there; see the 
documentation of|g_spawn_check_exit_status()| 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#g-spawn-check-exit-status>for 
how to use and interpret this. Note that it is invalid to 
pass|G_SPAWN_DO_NOT_REAP_CHILD| 
<https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#G-SPAWN-DO-NOT-REAP-CHILD:CAPS>in/|flags|/.

If an error occurs, no data is returned 
in/|standard_output|/,/|standard_error|/, or/|exit_status|/.
Yuri Pudgorodskiy Jan. 11, 2016, 12:08 p.m. UTC | #2
On 1/6/2016 4:50 PM, Denis V. Lunev wrote:
> hmmm....
>
> Yur,
>
> it seems that you have re-invented the wheel with
>
> gboolean 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gboolean>
> g_spawn_sync (/|const gchar 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
> *working_directory|/,
>               /|gchar 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
> **argv|/,
>               /|gchar 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
> **envp|/,
>               /|GSpawnFlags 
> <https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnFlags> 
> flags|/,
>               /|GSpawnChildSetupFunc 
> <https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnChildSetupFunc> 
> child_setup|/,
>               /|gpointer 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gpointer> 
> user_data|/,
>               /|gchar 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
> **standard_output|/,
>               /|gchar 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
> **standard_error|/,
>               /|gint 
> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gint> 
> *exit_status|/,
>               /|GError 
> <https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#GError> 
> **error|/);
>
>

Not exactly.

g_spawn_sync() wrapper cannot be used directly instead of my wrapper,
because it does not support redirected stdin which is required to send 
data to chpasswd.
g_spawn_async_with_pipes() can be used (with stdin), but it will not 
make code simpler.

Most of command-posix.c calls fork/execv directly, and so does the 
wrapper I wrote.
Denis V. Lunev Jan. 12, 2016, 6:33 a.m. UTC | #3
On 01/11/2016 03:08 PM, Yuriy Pudgorodskiy wrote:
> On 1/6/2016 4:50 PM, Denis V. Lunev wrote:
>> hmmm....
>>
>> Yur,
>>
>> it seems that you have re-invented the wheel with
>>
>> gboolean 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gboolean>
>> g_spawn_sync (/|const gchar 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
>> *working_directory|/,
>>               /|gchar 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
>> **argv|/,
>>               /|gchar 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
>> **envp|/,
>>               /|GSpawnFlags 
>> <https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnFlags> 
>> flags|/,
>>               /|GSpawnChildSetupFunc 
>> <https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#GSpawnChildSetupFunc> 
>> child_setup|/,
>>               /|gpointer 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gpointer> 
>> user_data|/,
>>               /|gchar 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
>> **standard_output|/,
>>               /|gchar 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gchar> 
>> **standard_error|/,
>>               /|gint 
>> <https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gint> 
>> *exit_status|/,
>>               /|GError 
>> <https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#GError> 
>> **error|/);
>>
>>
>
> Not exactly.
>
> g_spawn_sync() wrapper cannot be used directly instead of my wrapper,
> because it does not support redirected stdin which is required to send 
> data to chpasswd.
> g_spawn_async_with_pipes() can be used (with stdin), but it will not 
> make code simpler.
>
> Most of command-posix.c calls fork/execv directly, and so does the 
> wrapper I wrote.
>
>
this seems that you are right :)

Then nothing needs to be changed and patches are OK.

Den
diff mbox

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8fe708f..53e8d3b 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -75,6 +75,28 @@  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;
+}
+
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
 {
     const char *shutdown_flag;
@@ -1952,20 +1974,128 @@  int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
     return processed;
 }
 
+/* Helper to run command with input/output redirection,
+ * sending string to stdin and taking error message from
+ * stdout/err
+ */
+static void ga_run_program(const char *argv[],
+                           const char *in_str,
+                           const char *action, Error **errp)
+{
+    pid_t pid;
+    int status;
+    int infd[2] = { -1, -1 };
+    int outfd[2] = { -1, -1 };
+    char *str = NULL;
+    size_t len = 0;
+
+    if (in_str) {
+        if (pipe(infd) < 0) {
+            error_setg(errp, "cannot create pipe FDs");
+            goto out;
+        }
+    }
+
+    if (pipe(outfd) < 0) {
+        error_setg(errp, "cannot create pipe FDs");
+        goto out;
+    }
+
+    pid = fork();
+    if (pid == 0) {
+        /* child */
+        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]);
+
+        execve(argv[0], (char *const *)argv, environ);
+        _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;
+    }
+
+    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]);
+    }
+}
+
 void qmp_guest_set_user_password(const char *username,
                                  const char *password,
                                  bool crypted,
                                  Error **errp)
 {
-    Error *local_err = NULL;
     char *passwd_path = NULL;
-    pid_t pid;
-    int status;
-    int datafd[2] = { -1, -1 };
     char *rawpasswddata = NULL;
     size_t rawpasswdlen;
     char *chpasswddata = NULL;
-    size_t chpasswdlen;
+    const char *chpasswd_argv[] = { NULL /*path*/, "chpasswd", "-e", NULL };
 
     rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
     if (!rawpasswddata) {
@@ -1986,75 +2116,26 @@  void qmp_guest_set_user_password(const char *username,
     }
 
     chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
-    chpasswdlen = strlen(chpasswddata);
 
     passwd_path = g_find_program_in_path("chpasswd");
-
     if (!passwd_path) {
         error_setg(errp, "cannot find 'passwd' program in PATH");
         goto out;
     }
 
-    if (pipe(datafd) < 0) {
-        error_setg(errp, "cannot create pipe FDs");
-        goto out;
-    }
+    chpasswd_argv[0] = passwd_path;
 
-    pid = fork();
-    if (pid == 0) {
-        close(datafd[1]);
-        /* child */
-        setsid();
-        dup2(datafd[0], 0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        if (crypted) {
-            execle(passwd_path, "chpasswd", "-e", NULL, environ);
-        } else {
-            execle(passwd_path, "chpasswd", NULL, environ);
-        }
-        _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);
-    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;
+    /* set password for existed user */
+    if (!crypted) {
+        /* wipe -e option */
+        chpasswd_argv[2] = NULL;
     }
+    ga_run_program(chpasswd_argv, chpasswddata, "set user password", errp);
 
 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]);
-    }
 }
 
 static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,