maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Jan 17, Oleksandr Byelkin wrote:
> On 16.01.2016 21:56, Sergei Golubchik wrote:
> > On Nov 25, OleksandrByelkin wrote:
> >>
> >> diff --git a/include/mysql_com.h b/include/mysql_com.h
> >> index aa6ab0f..57092dc 100644
> >> --- a/include/mysql_com.h
> >> +++ b/include/mysql_com.h
> >> @@ -215,7 +216,8 @@ enum enum_server_command
> >>   /* Don't close the connection for a connection with expired password. */
> >>   #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
> >>   
> >> -#define CLIENT_PROGRESS  (1UL << 29)   /* Client support progress indicator */
> >> +#define CLIENT_PROGRESS_OBSOLETE  (1UL << 29)
> > ok
> >
> >> +#define CLIENT_PROGRESS           0
> > why is that?
> Because old flag will not work with the new server.

Right, but for binaries (already compiled libraries and clients), the
flag is (1UL << 29) - I mean, the number, not the "CLIENT_PROGRESS"
name. And I suggest it to continue to work, as I wrote in the review,
with something like

  if (client_capabilities & CLIENT_PROGRESS_OBSOLETE)
    client_capabilities|= MARIADB_CLIENT_PROGRESS;

As for the *name* CLIENT_PROGRESS - it only affects clients/libraries
that are compiled with the new MariaDB. And I'd rather see a compilation
failure in this case ("unknown symbol CLIENT_PROGRESS") than a silently
disabled feature.

So, I think, you don't need to keep old CLIENT_PROGRESS symbol, you can
safely remove it.

> >> +/* Client support mariaDB extended flags */
> >> +#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63)
> > how can this work, if client_flag and server_capabilities are
> > only 32-bit variables?
> it is not anymore (on the server) on non-connector client it will be 
> lost but who cares if it doesnot support it ((I removed support by your 
> request).

Oh, yes, right. This is only the server part of the patch.
Sorry.

> >> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> >> index 1240d5d..d07e0a4 100644
> >> --- a/sql/sql_acl.cc
> >> +++ b/sql/sql_acl.cc
> >> @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio,

> >> +    if (!(client_capabilities & CLIENT_MYSQL))
> >> +    {
> >> +      // it is client with mariadb extensions
> >> +      client_capabilities|= CLIENT_MYSQL;

Wait, why is this?

> >> +      ulonglong ext_client_capabilities=
> >> +        (((ulonglong)uint4korr(net->read_pos + 28)) << 32);
> >> +      if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS)
> >> +        client_capabilities|= ext_client_capabilities;
> >> +      else
> >> +      {
> >> +        DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, "
> >> +                             "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, "
> >> +                             "but MARIADB_CLIENT_EXTENDED_FLAGS is off. "
> >> +                             "flags: %llx ext flags %llx",
> >> +                             client_capabilities, ext_client_capabilities));
> >> +        return packet_error;
> > Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)?
> Server read or do not read more flags depend on it.

But !(client_capabilities & CLIENT_MYSQL) already tells you that.
It's a MariaDB client = it has extended flags.

> >> diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c
> >> index a1a52e8..31fca0f 100644
> >> --- a/tests/mysql_client_test.c
> >> +++ b/tests/mysql_client_test.c
> >> @@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
> >>   
> >>     myheader("test_mysql_insert_id");
> >>   
> >> -  rc= mysql_query(mysql, "drop table if exists t1");
> >> +  rc= mysql_query(mysql, "drop table if exists t1,t2");
> > separate commit, please
> ok, but it looks too late now, sorry.

If you haven't pushed to 10.2 yet, it's not late.
I'll ping you on irc and explain how to do it.

> >>     myquery(rc);
> >>     /* table without auto_increment column */
> >>     rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))");
> >> @@ -18700,7 +18700,8 @@ static void test_progress_reporting()
> >>     myheader("test_progress_reporting");
> >>   
> >>     conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0);
> >> -  DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS);
> >> +  if (!(conn->client_flag & CLIENT_PROGRESS))
> >> +    return;
> > what are you testing here?
> yes, I'll fix it.

I was only asking a question :)
I don't know what "it" is and what's wrong with it.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx
-- 
Vote for my Percona Live 2016 talks:
https://www.percona.com/live/data-performance-conference-2016/sessions/mariadb-connectors-fast-and-smart-new-protocol-optimizations#community-voting
https://www.percona.com/live/data-performance-conference-2016/sessions/mariadb-101-security-validation-authentication-encryption#community-voting