Message ID | 1452081699-11894-2-git-send-email-den@openvz.org |
---|---|
State | New |
Headers | show |
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|/.
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.
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 --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,