On Tue, Sep 30, 2014 at 04:24:49PM +0200, Tom Gundersen wrote:
> Hi David,
>
> Just a quick follow up on these two patches (sorry for the delay).
>
> On the one hand, we want this interface to be very basic and should
> probably not something we should extend to cover all sorts of things.
> On the other hand, this usecase (and similar ones where you have a
> real password, but maybe it is a one-time password, so you don't care
> about hiding it) seems very natural, and the patches are simple.
>
> I'll leave it up to Lennart to decide.
Yes, the patch looks OK, but like with the other one, please
rename do_echo to echo, and add an extra argument to the function instead
of adding a second function.
Zbyszek
> A couple of inline nitpicks:
>
> On Fri, Sep 19, 2014 at 12:09 PM, David Sommerseth <dav...@redhat.com> wrote:
> > This is a continuation of the patch "ask-password: Add --do-echo to enable
> > echoing the user input".
> >
> > This allows user input going via the ask-password-agent to be echoed
> > as well, if the --do-echo argument is provided to systemd-ask-password.
> >
> > Again it was preferred to add a new function, ask_password_agent_echo(),
> > over modifying the ask_password_agent() API to make the use case clearer
> > and keep backwards compatibility with applications depending on
> > ask_password_agent().
> >
> > Signed-off-by: David Sommerseth <dav...@redhat.com>
>
> No need for s-o-b in systemd.
>
> > ---
> > src/ask-password/ask-password.c | 5 ++++-
> > src/shared/ask-password-api.c | 16 +++++++++++++++-
> > src/shared/ask-password-api.h | 2 ++
> > src/tty-ask-password-agent/tty-ask-password-agent.c | 6 ++++--
> > 4 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ask-password/ask-password.c
> > b/src/ask-password/ask-password.c
> > index c77cc66..c6744b9 100644
> > --- a/src/ask-password/ask-password.c
> > +++ b/src/ask-password/ask-password.c
> > @@ -179,7 +179,10 @@ int main(int argc, char *argv[]) {
> > } else {
> > char **l;
> >
> > - if ((r = ask_password_agent(arg_message, arg_icon, arg_id,
> > timeout, arg_accept_cached, &l)) >= 0) {
> > + r = arg_do_echo ? ask_password_agent_echo(arg_message,
> > arg_icon, arg_id, timeout, arg_accept_cached, &l)
> > + : ask_password_agent(arg_message, arg_icon,
> > arg_id, timeout, arg_accept_cached, &l);
> > +
> > + if (r >= 0) {
> > char **p;
> >
> > STRV_FOREACH(p, l) {
> > diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
> > index 283bfc2..39c122e 100644
> > --- a/src/shared/ask-password-api.c
> > +++ b/src/shared/ask-password-api.c
> > @@ -311,12 +311,13 @@ fail:
> > return r;
> > }
> >
> > -int ask_password_agent(
> > +static int __ask_password_agent(
> > const char *message,
> > const char *icon,
> > const char *id,
> > usec_t until,
> > bool accept_cached,
> > + bool do_echo,
> > char ***_passphrases) {
> >
> > enum {
> > @@ -378,10 +379,12 @@ int ask_password_agent(
> > "PID="PID_FMT"\n"
> > "Socket=%s\n"
> > "AcceptCached=%i\n"
> > + "DoEcho=%i\n"
>
> I would probably rename this to just "Echo"
>
> > "NotAfter="USEC_FMT"\n",
> > getpid(),
> > socket_name,
> > accept_cached ? 1 : 0,
> > + do_echo ? 1 : 0,
> > until);
> >
> > if (message)
> > @@ -557,6 +560,17 @@ finish:
> > return r;
> > }
> >
> > +int ask_password_agent(const char *message, const char *icon, const char
> > *id,
> > + usec_t until, bool accept_cached, char
> > ***_passphrases) {
> > + return __ask_password_agent(message, icon, id, until,
> > accept_cached, false, _passphrases);
> > +}
> > +
> > +int ask_password_agent_echo(const char *message, const char *icon, const
> > char *id,
> > + usec_t until, bool accept_cached, char
> > ***_passphrases) {
> > + return __ask_password_agent(message, icon, id, until,
> > accept_cached, true, _passphrases);
> > +}
> > +
> > +
> > int ask_password_auto(const char *message, const char *icon, const char
> > *id,
> > usec_t until, bool accept_cached, char
> > ***_passphrases) {
> > assert(message);
> > diff --git a/src/shared/ask-password-api.h b/src/shared/ask-password-api.h
> > index c3dde63..d467398 100644
> > --- a/src/shared/ask-password-api.h
> > +++ b/src/shared/ask-password-api.h
> > @@ -28,6 +28,8 @@ int ask_password_tty_echo(const char *message, usec_t
> > until, const char *flag_fi
> >
> > int ask_password_agent(const char *message, const char *icon, const char
> > *id,
> > usec_t until, bool accept_cached, char
> > ***_passphrases);
> > +int ask_password_agent_echo(const char *message, const char *icon, const
> > char *id,
> > + usec_t until, bool accept_cached, char
> > ***_passphrases);
> >
> > int ask_password_auto(const char *message, const char *icon, const char
> > *id,
> > usec_t until, bool accept_cached, char
> > ***_passphrases);
> > diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c
> > b/src/tty-ask-password-agent/tty-ask-password-agent.c
> > index 8a02fb0..90bbd1e 100644
> > --- a/src/tty-ask-password-agent/tty-ask-password-agent.c
> > +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c
> > @@ -214,7 +214,7 @@ static int parse_password(const char *filename, char
> > **wall) {
> > _cleanup_free_ char *socket_name = NULL, *message = NULL, *packet
> > = NULL;
> > uint64_t not_after = 0;
> > unsigned pid = 0;
> > - bool accept_cached = false;
> > + bool accept_cached = false, do_echo = false;
> >
> > const ConfigTableItem items[] = {
> > { "Ask", "Socket", config_parse_string, 0,
> > &socket_name },
> > @@ -222,6 +222,7 @@ static int parse_password(const char *filename, char
> > **wall) {
> > { "Ask", "Message", config_parse_string, 0,
> > &message },
> > { "Ask", "PID", config_parse_unsigned, 0, &pid
> > },
> > { "Ask", "AcceptCached", config_parse_bool, 0,
> > &accept_cached },
> > + { "Ask", "DoEcho", config_parse_bool, 0,
> > &do_echo },
> > {}
> > };
> >
> > @@ -314,7 +315,8 @@ static int parse_password(const char *filename, char
> > **wall) {
> > return tty_fd;
> > }
> >
> > - r = ask_password_tty(message, not_after, filename,
> > &password);
> > + r = do_echo ? ask_password_tty_echo(message,
> > not_after, filename, &password)
> > + : ask_password_tty(message, not_after,
> > filename, &password);
> >
> > if (arg_console) {
> > safe_close(tty_fd);
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
