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.drobys...@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 :|
