diff mbox series

[v2,7/7] qga/commands-posix: qmp_guest_set_user_password: use ga_run_command helper

Message ID 20240301172858.665135-8-andrey.drobyshev@virtuozzo.com
State New
Headers show
Series qga/commands-posix: replace code duplicating commands with a helper | expand

Commit Message

Andrey Drobyshev March 1, 2024, 5:28 p.m. UTC
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(-)

Comments

Daniel P. Berrangé March 5, 2024, 6:38 p.m. UTC | #1
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.

> +    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
Andrey Drobyshev March 15, 2024, 11:06 a.m. UTC | #2
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.
diff mbox series

Patch

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__ */