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.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.
>
> [...]
Sure, let's get rid of them. Thanks.
