Re: [Qemu-devel] [PATCH 1/1] qga: guest-set-user-password - added ability to create new user

[Eric Blake]

On 12/20/2015 11:40 PM, Denis V. Lunev wrote:
> From: Yuri Pudgorodskiy <address@hidden>
> 
> Added optional 'create' flag to guest-set-user-password command.
> When it is specified, a new user will be created if it is not
> exists yet.

s/is not exists/does not exist/

> 
> The option to the existing command is added as password for newly created
> user should be set as specified.
> 
> Signed-off-by: Yuri Pudgorodskiy <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Michael Roth <address@hidden>
> ---
>  qga/commands-posix.c | 36 ++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c | 25 ++++++++++++++++++++++++-
>  qga/qapi-schema.json |  3 ++-
>  3 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c

> @@ -1993,6 +1995,40 @@ void qmp_guest_set_user_password(const char *username,
>          goto out;
>      }
>  
> +    /* create new user if requested */
> +    if (has_create && create) {
> +        pid = fork();
> +        if (pid == 0) {
> +            char *str = g_shell_quote(username);
> +            char *cmd = g_strdup_printf("id %s || useradd -m %s", str, str);

useradd is Linux-specific; should we be trying harder to make this
command portable to all POSIX-y guests?

> +            setsid();
> +            reopen_fd_to_null(0);
> +            reopen_fd_to_null(1);
> +            reopen_fd_to_null(2);
> +            execle("/bin/sh", "sh", "-c", cmd, NULL, environ);

By redirecting stderr to /dev/null, you've lost any error messages that
useradd tries to report...

> +            _exit(EXIT_FAILURE);
> +        } else if (pid < 0) {
> +            error_setg_errno(errp, errno, "failed to create child process");
> +            goto out;
> +        }
> +
> +        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 add new user");

...and replaced it with a less-helpful message. Should you try harder to
pass through the real reason for failure?

> +++ b/qga/qapi-schema.json
> @@ -787,6 +787,7 @@
>  # @username: the user account whose password to change
>  # @password: the new password entry string, base64 encoded
>  # @crypted: true if password is already crypt()d, false if raw
> +# @create: #optional user will be created if not exists (since 2.6)

s/if not exists/if it does not exist/

may want to mention that it defaults to false

>  #
>  # If the @crypted flag is true, it is the caller's responsibility
>  # to ensure the correct crypt() encryption scheme is used. This
> @@ -806,7 +807,7 @@
>  # Since 2.3
>  ##
>  { 'command': 'guest-set-user-password',
> -  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
> +  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool', 
> '*create': 'bool' } }

Long line; please wrap to keep things under 80 columns.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature