qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD


From: Alexander Ivanov
Subject: Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD
Date: Fri, 30 Sep 2022 15:11:52 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0

Hi

On 30.09.2022 10:19, Marc-André Lureau wrote:

        
Caution: This is an external email and has a suspicious subject or content. Please take care when clicking links or opening attachments. When in doubt, contact your IT Department


Hi

On Thu, Sep 29, 2022 at 6:29 PM Alexander Ivanov <alexander.ivanov@virtuozzo.com> wrote:


    On 29.09.2022 13:22, Marc-André Lureau wrote:
    >
    >
    > Caution: This is an external email and has a suspicious subject or
    > content. Please take care when clicking links or opening
    attachments.
    > When in doubt, contact your IT Department
    >
    >
    > Hi Alexander
    >
    > On Thu, Sep 29, 2022 at 12:52 PM Alexander Ivanov
    > <alexander.ivanov@virtuozzo.com> wrote:
    >
    >     Move qmp_guest_set_user_password() from __linux__ condition to
    >     (__linux__ || __FreeBSD__) condition. Add command and arguments
    >     for password setting in FreeBSD.
    >
    >     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
    >
    >
    > Could you explain why you need to move the code?
    >
    > You could instead have a top declaration?
    Now qmp_guest_set_user_password()is under #ifdef __linux__. I want to
    use this function for FreeBSD too, so I moved it under (__linux__ ||
    __FreeBSD__) condition. I could create another function for
    FreeBSD but


Why not make the following change?
I thought it is a bad idea to multiply pieces of code under the same arch conditions. OK, I will change the code in this way. Thank you.

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 88e0d0fe24..78a345c9f3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2122,7 +2122,9 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)

     return processed;
 }
+#endif /* linux */

+#if defined(__linux__) || defined(__FreeBSD__)
 void qmp_guest_set_user_password(const char *username,
                                  const char *password,
                                  bool crypted,
@@ -2227,7 +2229,9 @@ out:
         close(datafd[1]);
     }
 }
+#endif /* linux || freebsd */

+#if defined(__linux__)
 static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
                                int size, Error **errp)
 {

    it would lead to code duplication. Unfortunately I don't see
    another way
    to do it except to add (__linux__ || __FreeBSD__) condition, cut the
    function and paste in this condition. If you could suggest another
    way I
    would happy.
    > If it's really required, please split the patch in move + additions.
    Just moving the function without other changes leads to
    unbuildable code
    in FreeBSD. I thought it's worse than moving and changing at the same
    time. I split the patch if you prefer.

    >
    >     ---
    >      qga/commands-posix.c | 223
    >     +++++++++++++++++++++++--------------------
    >      1 file changed, 118 insertions(+), 105 deletions(-)
    >
    >     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
    >     index 88e0d0fe24..6ce894ca6e 100644
    >     --- a/qga/commands-posix.c
    >     +++ b/qga/commands-posix.c
    >     @@ -2123,111 +2123,6 @@ int64_t
    >     qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error
    **errp)
    >          return processed;
    >      }
    >
    >     -void qmp_guest_set_user_password(const char *username,
    >     -                                 const char *password,
    >     -                                 bool crypted,
    >     -                                 Error **errp)
    >     -{
    >     -    Error *local_err = NULL;
    >     -    char *passwd_path = NULL;
    >     -    pid_t pid;
    >     -    int status;
    >     -    int datafd[2] = { -1, -1 };
    >     -    char *rawpasswddata = NULL;
    >     -    size_t rawpasswdlen;
    >     -    char *chpasswddata = NULL;
    >     -    size_t chpasswdlen;
    >     -
    >     -    rawpasswddata = (char *)qbase64_decode(password, -1,
    >     &rawpasswdlen, errp);
    >     -    if (!rawpasswddata) {
    >     -        return;
    >     -    }
    >     -    rawpasswddata = g_renew(char, rawpasswddata,
    rawpasswdlen + 1);
    >     -    rawpasswddata[rawpasswdlen] = '\0';
    >     -
    >     -    if (strchr(rawpasswddata, '\n')) {
    >     -        error_setg(errp, "forbidden characters in raw
    password");
    >     -        goto out;
    >     -    }
    >     -
    >     -    if (strchr(username, '\n') ||
    >     -        strchr(username, ':')) {
    >     -        error_setg(errp, "forbidden characters in username");
    >     -        goto out;
    >     -    }
    >     -
    >     -    chpasswddata = g_strdup_printf("%s:%s\n", username,
    >     rawpasswddata);
    >     -    chpasswdlen = strlen(chpasswddata);
    >     -
    >     -    passwd_path = g_find_program_in_path("chpasswd");
    >     -
    >     -    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);
    >     -
    >     -        if (crypted) {
    >     -            execl(passwd_path, "chpasswd", "-e", NULL);
    >     -        } else {
    >     -            execl(passwd_path, "chpasswd", NULL);
    >     -        }
    >     -        _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);
    >     -    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]);
    >     -    }
    >     -}
    >     -
    >      static void ga_read_sysfs_file(int dirfd, const char *pathname,
    >     char *buf,
    >                                     int size, Error **errp)
    >      {
    >     @@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo
    >     *qmp_guest_get_memory_block_info(Error **errp)
    >
    >      #endif
    >
    >     +#if defined(__linux__) || defined(__FreeBSD__)
    >     +void qmp_guest_set_user_password(const char *username,
    >     +                                 const char *password,
    >     +                                 bool crypted,
    >     +                                 Error **errp)
    >     +{
    >     +    Error *local_err = NULL;
    >     +    char *passwd_path = NULL;
    >     +    pid_t pid;
    >     +    int status;
    >     +    int datafd[2] = { -1, -1 };
    >     +    char *rawpasswddata = NULL;
    >     +    size_t rawpasswdlen;
    >     +    char *chpasswddata = NULL;
    >     +    size_t chpasswdlen;
    >     +
    >     +    rawpasswddata = (char *)qbase64_decode(password, -1,
    >     &rawpasswdlen, errp);
    >     +    if (!rawpasswddata) {
    >     +        return;
    >     +    }
    >     +    rawpasswddata = g_renew(char, rawpasswddata,
    rawpasswdlen + 1);
    >     +    rawpasswddata[rawpasswdlen] = '\0';
    >     +
    >     +    if (strchr(rawpasswddata, '\n')) {
    >     +        error_setg(errp, "forbidden characters in raw
    password");
    >     +        goto out;
    >     +    }
    >     +
    >     +    if (strchr(username, '\n') ||
    >     +        strchr(username, ':')) {
    >     +        error_setg(errp, "forbidden characters in username");
    >     +        goto out;
    >     +    }
    >     +
    >     +#ifdef __FreeBSD__
    >     +    chpasswddata = g_strdup(rawpasswddata);
    >     +    passwd_path = g_find_program_in_path("pw");
    >     +#else
    >     +    chpasswddata = g_strdup_printf("%s:%s\n", username,
    >     rawpasswddata);
    >     +    passwd_path = g_find_program_in_path("chpasswd");
    >     +#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);
    >     +    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]);
    >     +    }
    >     +}
    >     +#endif
    >     +
    >      #ifdef HAVE_GETIFADDRS
    >      static GuestNetworkInterface *
    >      guest_find_interface(GuestNetworkInterfaceList *head,
    >     --
    >     2.34.1
    >
    >
    >
    >
    > --
    > Marc-André Lureau



--
Marc-André Lureau



reply via email to

[Prev in Thread] Current Thread [Next in Thread]