Hi,
On Mon, Oct 10, 2016 at 07:10:30PM -0700, Kevin J. McCarthy wrote:
> On Sat, Aug 13, 2016 at 05:30:53PM -0400, Damien Riegel wrote:
> > In both cases, the value of debuglevel is wrongly used: the debug string
> > is printed only if debuglevel is below IMAP_LOG_PASS or
> > MUTT_SOCK_LOG_FULL. But the debug is supposed to be more verbose as
> > debuglevel increases.
>
> Sorry, but the usages are actually fine, as explained in more detail
> below.
>
> > diff --git a/imap/auth_login.c b/imap/auth_login.c
> > index 3e40d4d..83c0932 100644
> > --- a/imap/auth_login.c
> > +++ b/imap/auth_login.c
> > @@ -50,12 +50,8 @@ imap_auth_res_t imap_auth_login (IMAP_DATA* idata, const
> > char* method)
> > imap_quote_string (q_pass, sizeof (q_pass), idata->conn->account.pass);
> >
> > #ifdef DEBUG
> > - /* don't print the password unless we're at the ungodly debugging level
> > - * of 5 or higher */
> > -
> > - if (debuglevel < IMAP_LOG_PASS)
> > - dprint (2, (debugfile, "Sending LOGIN command for %s...\n",
> > - idata->conn->account.user));
> > + dprint (2, (debugfile, "Sending LOGIN command for %s...\n",
> > + idata->conn->account.user));
> > #endif
>
> What is happening is that if the debuglevel is less than 5, it *instead*
> prints the above message.
Maybe you disagree, but I think that the message should not be replaced
by another one when the log level is increased, it should be printed in
addition to the other.
> When the login is performed, the flag IMAP_CMD_PASS is passed through:
> imap_auth_login() -> imap_exec() -> cmd_start()
> cmd_start() then bumps the dbg level to IMAP_LOG_PASS if that flag is set,
> passing it through to mutt_socket_write_d().
> The mutt_socket_write_d() then uses that debug level for its dprint.
>
>
> > diff --git a/pop_auth.c b/pop_auth.c
> > index 8d8650e..8619735 100644
> > --- a/pop_auth.c
> > +++ b/pop_auth.c
> > @@ -270,12 +270,7 @@ static pop_auth_res_t pop_auth_user (POP_DATA
> > *pop_data, const char *method)
> > if (ret == 0)
> > {
> > snprintf (buf, sizeof (buf), "PASS %s\r\n",
> > pop_data->conn->account.pass);
> > - ret = pop_query_d (pop_data, buf, sizeof (buf),
> > -#ifdef DEBUG
> > - /* don't print the password unless we're at the ungodly debugging level
> > */
> > - debuglevel < MUTT_SOCK_LOG_FULL ? "PASS *\r\n" :
> > -#endif
> > - NULL);
> > + ret = pop_query_d (pop_data, buf, sizeof (buf), "PASS *\r\n");
>
> pop_query_d() looks at the last parameter to set the debug level for
> mutt_socket_write_d(), as above. If it's non-null, pop_query_d() logs
> that message *instead* and bumps the mutt_socket_write_d() dbg parameter
> higher.
>
> So this hunk has actually removed the ability to log the actual pass
> command even at level 4.
Okay, I get the logic now. Wouldn't it be simpler if the last argument
of pop_query_d actually was the log level required to print the query,
and we could drop this substitution altogether.
For IMAP, the pass is printed at level 5, and at level 4 and 5 for
POP. Maybe we could also make that consistent and only display the
password at level 5 for both?
--
Damien
