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.3On 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 :|
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.
© 2016 - 2026 Red Hat, Inc.