Re: [Qemu-devel] [PATCH 1/2] create ga_run_program() helper for guest-set-user-password

["Denis V. Lunev" <den@openvz.org>]

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|/.