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

[Andrey Drobyshev]

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.