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

[Denis V. Lunev]

On 12/21/2015 05:00 PM, Eric Blake wrote:
> 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?
this code is inside #if defined(__linux__)
as implemented by the original author.


> > +            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.

the rest is clear. We will try.