maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Timour!

Just few comments

On Jun 25, timour@xxxxxxxxxxxx wrote:
> ------------------------------------------------------------
> revno: 3517
> revision-id: timour@xxxxxxxxxxxx-20130621115451-tyxgeeroqyxwz7gn
> parent: monty@xxxxxxxxxxxx-20130623091543-zqu5dvhry6okna6g
> committer: timour@xxxxxxxxxxxx
> branch nick: 10.0-perf-schema-merge
> timestamp: Fri 2013-06-21 14:54:51 +0300
> message:
>   MySQL 5.6.10 performance schema: merge of host_cache table

> === modified file 'sql/hostname.cc'
> --- a/sql/hostname.cc	2013-01-15 18:13:32 +0000
> +++ b/sql/hostname.cc	2013-06-21 11:54:51 +0000
> @@ -24,7 +24,7 @@
>    Hostnames are checked with reverse name lookup and checked that they
>    doesn't resemble an IP address.
>  */
> -
> +#include "my_global.h"
>  #include "sql_priv.h"
>  #include "hostname.h"
>  #include "my_global.h"

you don't need to include my_global.h twice here

> === modified file 'sql/sql_acl.cc'
> --- a/sql/sql_acl.cc	2013-06-17 23:01:34 +0000
> +++ b/sql/sql_acl.cc	2013-06-21 11:54:51 +0000
> @@ -8730,7 +8737,6 @@ static int server_mpvio_read_packet(MYSQ
>  err:
>    if (mpvio->status == MPVIO_EXT::FAILURE)
>    {
> -    inc_host_errors(mpvio->thd->security_ctx->ip);
>      if (!mpvio->thd->is_error())
>      {
>        if (mpvio->make_it_fail)
> @@ -8890,6 +8896,10 @@ static int do_auth_once(THD *thd, const
>    else
>    {
>      /* Server cannot load the required plugin. */
> +    Host_errors errors;
> +    errors.m_no_auth_plugin= 1;
> +    // merge-todo: struct MPVIO_EXT’ has no member named ‘ip’
> +    // inc_host_errors(mpvio->ip, &errors);

see above, the line you've removed in a previous chunk, used

  inc_host_errors(mpvio->thd->security_ctx->ip);

>      my_error(ER_PLUGIN_IS_NOT_LOADED, MYF(0), auth_plugin_name->str);
>      res= CR_ERROR;
>    }
> @@ -8913,8 +8923,6 @@ static int do_auth_once(THD *thd, const
>    Perform the handshake, authorize the client and update thd sctx variables.
>  
>    @param thd                     thread handle
> -  @param connect_errors          number of previous failed connect attemps
> -                                 from this host

really? you've removed it from the comment, but not from the function,
so now acl_authenticate() has one undocumented argument.

>    @param com_change_user_pkt_len size of the COM_CHANGE_USER packet
>                                   (without the first, command, byte) or 0
>                                   if it's not a COM_CHANGE_USER (that is, if
> @@ -9017,8 +9025,27 @@ bool acl_authenticate(THD *thd, uint con
>  
>    if (res > CR_OK && mpvio.status != MPVIO_EXT::SUCCESS)
>    {
> +    Host_errors errors;
>      DBUG_ASSERT(mpvio.status == MPVIO_EXT::FAILURE);
> -
> +    switch (res)
> +    {
> +    case CR_AUTH_PLUGIN_ERROR:
> +      errors.m_auth_plugin= 1;
> +      break;
> +    case CR_AUTH_HANDSHAKE:
> +      errors.m_handshake= 1;
> +      break;
> +    case CR_AUTH_USER_CREDENTIALS:
> +      errors.m_authentication= 1;
> +      break;
> +    case CR_ERROR:
> +    default:
> +      /* Unknown of unspecified auth plugin error. */
> +      errors.m_auth_plugin= 1;
> +      break;
> +    }
> +    // merge-todo: struct MPVIO_EXT’ has no member named ‘ip’
> +    // inc_host_errors(mpvio.ip, &errors);

same as above.

And personally, I'd rather make a method errors.set_error(res); and move
the switch inside. But you may decide to stay closer to 5.6 code.

>      if (!thd->is_error())
>        login_failed_error(thd);
>      DBUG_RETURN(1);
> @@ -9256,23 +9283,20 @@ static int native_password_authenticate(
>  #endif
>  
>    if (pkt_len == 0) /* no password */
> -    DBUG_RETURN(info->auth_string[0] ? CR_ERROR : CR_OK);
> +    DBUG_RETURN(mpvio->acl_user->salt_len != 0 ? CR_AUTH_USER_CREDENTIALS : CR_OK);

Why did you change the condition?
Our pluggable auth code is not identical to MySQL's, so not all
changes can be copied verbatim from there.

I think the old condition was ok.

>    info->password_used= PASSWORD_USED_YES;
>    if (pkt_len == SCRAMBLE_LENGTH)
>    {
>      if (!mpvio->acl_user->salt_len)
> -      DBUG_RETURN(CR_ERROR);
> +      DBUG_RETURN(CR_AUTH_USER_CREDENTIALS);
>  
> -    if (check_scramble(pkt, thd->scramble, mpvio->acl_user->salt))
> -      DBUG_RETURN(CR_ERROR);
> -    else
> -      DBUG_RETURN(CR_OK);
> +    DBUG_RETURN(check_scramble(pkt, thd->scramble, mpvio->acl_user->salt) ?
> +                CR_AUTH_USER_CREDENTIALS : CR_OK);

could you change it back please and only replace the error code?
if() is easier to debug, than a function call inside DBUG_RETURN().
I suppose that's why we have it that way.

>    }
>  
> -  inc_host_errors(mpvio->thd->security_ctx->ip);
>    my_error(ER_HANDSHAKE_ERROR, MYF(0));
> -  DBUG_RETURN(CR_ERROR);
> +  DBUG_RETURN(CR_AUTH_HANDSHAKE);
>  }

Regards,
Sergei