Re: [Maria-developers] Review of 5.2 pluggable-auth tree
Hi! 26 марта 2010, в 12:12, Sergei Golubchik написал(а): [skip] + #ifdef _WIN32 In the rest of my_global.h we use __WIN__ Wouldn't it be better to use the same constant everywhere? Yes, it would. And this constant should be _WIN32, __WIN__ was historically used, but it's incorrect - as was explained by our windows developers. I've just used the recommended symbol. If we both was confused by this maybe it has sense t put comment here... [skip]___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Review of 5.2 pluggable-auth tree
Hi, Oleksandr! On Mar 29, Oleksandr Byelkin wrote: [skip] + #ifdef _WIN32 In the rest of my_global.h we use __WIN__ Wouldn't it be better to use the same constant everywhere? Yes, it would. And this constant should be _WIN32, __WIN__ was historically used, but it's incorrect - as was explained by our windows developers. I've just used the recommended symbol. If we both was confused by this maybe it has sense t put comment here... where ? every time _WIN32 is used ? No need to be confused, see for example http://predef.sourceforge.net/preos.html#sec24 or http://msdn.microsoft.com/en-us/library/b0084kay%28VS.80%29.aspx Regards, Sergei ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Review of 5.2 pluggable-auth tree
29.03.2010 09:53, Sergei Golubchik пишет: Hi, Oleksandr! On Mar 29, Oleksandr Byelkin wrote: [skip] + #ifdef _WIN32 In the rest of my_global.h we use __WIN__ Wouldn't it be better to use the same constant everywhere? Yes, it would. And this constant should be _WIN32, __WIN__ was historically used, but it's incorrect - as was explained by our windows developers. I've just used the recommended symbol. If we both was confused by this maybe it has sense t put comment here... where ? every time _WIN32 is used ? No need to be confused, see for example http://predef.sourceforge.net/preos.html#sec24 or http://msdn.microsoft.com/en-us/library/b0084kay%28VS.80%29.aspx in other cases difference between win32/64 is not so important IMHO, so here comment would be good. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Review of 5.2 pluggable-auth tree
Hi, Monty!
On Mar 26, Sergei Golubchik wrote:
Hi, Monty!
See the answers below:
(no reply means ok, changed)
On Mar 25, Michael Widenius wrote:
And here's a patch that implements your review comments.
Regards,
Sergei
=== modified file 'client/mysql.cc'
*** client/mysql.cc 2010-03-15 17:35:32 +
--- client/mysql.cc 2010-03-26 11:32:46 +
***
*** 4273,4296
const char *prompt,
char *buf, int buf_len)
{
! int ch;
! char *s=buf, *end=buf+buf_len-1;
! fputs([mysql] , stdout);
fputs(prompt, stdout);
fputs( , stdout);
if (type == 2) /* password */
{
s= get_tty_password();
! strncpy(buf, s, buf_len);
my_free(s, MYF(0));
}
else
{
! for (ch= fgetc(stdin); s end ch != '\n' ch != EOF; ch=
fgetc(stdin))
! *s++= ch;
! *s=0;
}
return buf;
--- 4273,4296
const char *prompt,
char *buf, int buf_len)
{
! char *s=buf;
! fputs([mariadb] , stdout);
fputs(prompt, stdout);
fputs( , stdout);
if (type == 2) /* password */
{
s= get_tty_password();
! strnmov(buf, s, buf_len);
! buf[buf_len-1]= 0;
my_free(s, MYF(0));
}
else
{
! fgets(buf, buf_len-1, stdin);
! if (buf[0] (s= strend(buf))[-1] == '\n')
! s[-1]= 0;
}
return buf;
=== modified file 'libmysqld/lib_sql.cc'
*** libmysqld/lib_sql.cc2010-03-15 17:35:32 +
--- libmysqld/lib_sql.cc2010-03-26 11:32:46 +
***
*** 415,421
int emb_read_change_user_result(MYSQL *mysql)
{
mysql-net.read_pos= (uchar*); // fake an OK packet
! return mysql_errno(mysql) ? packet_error : 1;
}
MYSQL_METHODS embedded_methods=
--- 415,421
int emb_read_change_user_result(MYSQL *mysql)
{
mysql-net.read_pos= (uchar*); // fake an OK packet
! return mysql_errno(mysql) ? packet_error : 1 /* length of the OK packet */;
}
MYSQL_METHODS embedded_methods=
=== modified file 'plugin/auth/auth_socket.c'
*** plugin/auth/auth_socket.c 2010-03-11 12:33:22 +
--- plugin/auth/auth_socket.c 2010-03-26 11:32:46 +
***
*** 1,5
/* Copyright (C) 2010 Monty Program Ab
! /* Copyright (C) 2010 Monty Program Ab
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
--- 1,5
/* Copyright (C) 2010 Monty Program Ab
! /* Copyright (C) 2010 Sergei Golubchik and Monty Program Ab
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
=== modified file 'plugin/auth/dialog.c'
*** plugin/auth/dialog.c2010-03-11 12:33:22 +
--- plugin/auth/dialog.c2010-03-26 11:32:46 +
***
*** 1,5
/* Copyright (C) 2010 Monty Program Ab
! /* Copyright (C) 2010 Monty Program Ab
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
--- 1,5
/* Copyright (C) 2010 Monty Program Ab
! /* Copyright (C) 2010 Sergei Golubchik and Monty Program Ab
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
***
*** 236,242
We send the password, assuming the client knows what its doing.
(in other words, the dialog plugin should be only set as a default
authentication plugin on the client if the first question
! asks for a password - which will be sent in cleat text, by the way)
*/
reply= mysql-passwd;
}
--- 236,242
We send the password, assuming the client knows what its doing.
(in other words, the dialog plugin should be only set as a default
authentication plugin on the client if the first question
! asks for a password - which will be sent in clear text, by the way)
*/
reply= mysql-passwd;
}
=== modified file 'sql-common/client.c'
*** sql-common/client.c 2010-03-11 12:33:22 +
--- sql-common/client.c 2010-03-26 11:32:46 +
***
*** 109,114
#include sql_common.h
#include mysql/client_plugin.h
uintmysql_port=0;
char*mysql_unix_port= 0;
const char *unknown_sqlstate= HY000;
--- 109,117
#include sql_common.h
#include mysql/client_plugin.h
+ #define native_password_plugin_name mysql_native_password
+ #define old_password_plugin_namemysql_old_password
+
uintmysql_port=0;
char*mysql_unix_port= 0;
const char *unknown_sqlstate= HY000;
***
*** 1047,1059
Re: [Maria-developers] Review of 5.2 pluggable-auth tree
Hi, Monty!
See the answers below:
(no reply means ok, changed)
On Mar 25, Michael Widenius wrote:
=== modified file 'client/mysql.cc'
+ /**
+ An example of mysql_authentication_dialog_ask callback.
+
+ The C function with the name mysql_authentication_dialog_ask, if
exists,
+ will be used by the dialog client authentication plugin when user
+ input is needed. This function should be of
mysql_authentication_dialog_ask_t
+ type. If the function does not exists, a built-in implementation will be
+ used.
+
+ @param mysql mysql
+ @param type type of the input
+ 1 - normal string input
+ 2 - password string
+ @param prompt prompt
+ @param bufa buffer to store the use input
+ @param buf_lenthe length of the buffer
+
+ @retval a pointer to the user input string.
+ It may be equal to 'buf' or to 'mysql-password'.
+ In all other cases it is assumed to be an
allocated
+ string, and the dialog plugin will free() it.
+ */
+ extern C char *mysql_authentication_dialog_ask(MYSQL *mysql, int type,
I would prefer to have type as my_bool as there is only two options
for it and rename it to 'ask_for_password'. This would make the code
more self_explanatory.
it cannot be my_bool because my_bool is internal MySQL typedef, not part
of the API.
and it's not really ask_for_password it's just ask for anything.
Password, your dog's name, the mainden name of your neighbour's wife -
whatever the plugin wants to ask for.
+ const char *prompt,
+ char *buf, int buf_len)
+ {
+ int ch;
+ char *s=buf, *end=buf+buf_len-1;
+
+ fputs([mysql] , stdout);
=== modified file 'include/my_global.h'
*** include/my_global.h 2009-12-03 11:19:05 +
--- include/my_global.h 2010-02-19 08:18:09 +
*** int __void__;
*** 578,583
--- 578,591
#define IF_VALGRIND(A,B) (B)
#endif
+ #ifdef _WIN32
In the rest of my_global.h we use __WIN__
Wouldn't it be better to use the same constant everywhere?
Yes, it would. And this constant should be _WIN32, __WIN__ was
historically used, but it's incorrect - as was explained by our windows
developers. I've just used the recommended symbol.
+ #define SO_EXT .dll
+ #elif defined(__APPLE__)
+ #define SO_EXT .dylib
+ #else
+ #define SO_EXT .so
+ #endif
+ /** the max allowed length for a user name */
+ #define MYSQL_USERNAME_LENGTH 48
Shouldn't this be the define USERNAME_LENGTH ?
Otherwise there will be a crash if someone redefines USERNAME_CHAR_LENGTH
USERNAME_LENGTH is in mysql_com.h
Instead of including one file in another, I've ensured that there two
defines are exactly equal with compile time asserts. I'm sure you've
seen them when you've reviewed a little further down.
*** void init_embedded_mysql(MYSQL *mysql, i
*** 584,589
--- 582,588
THD *thd = (THD *)mysql-thd;
thd-mysql= mysql;
mysql-server_version= server_version;
+ mysql-client_flag= client_flag;
init_alloc_root(mysql-field_alloc, 8192, 0);
}
Was this a bug in the old code ?
I don't remember anymore. May be mysql-client_flag were not really used
in embedded. Or may be there was a bug.
+ mysql-client_flag|= mysql-options.client_flag;
+ mysql-client_flag|= CLIENT_CAPABILITIES;
The code would be easier to read (and a bit faster/shorter) if you have
client_flag as a static variable and set mysql-client_flag from this
when all options are set.
don't worry, compiler does pretty much the same automatically at -O3,
I've just checked.
+ if (data_len)
Add a comment that data_len is always password length here.
it's not the length of the password, it's the length of whatever data
client plugin wants to send.
+ /* otherwise read the data */
+ pkt_len= (*mysql-methods-read_change_user_result)(mysql);
+ mpvio-last_read_packet_len= pkt_len;
+ *buf= mysql-net.read_pos;
+
+ /* was it a request to change plugins ? */
+ if (**buf == 254)
+ return (int)packet_error; /* if yes, this plugin shan't continue */
shan't - can't
The plugin shall not continue.
+ /**
+ vio-write_packet() callback method for client authentication plugins
+
+ This function is called by a client authentication plugin, when it wants
+ to send data to the server.
+
+ It transparently wraps the data into a change user or authentication
+ handshake packet, if neccessary.
+ */
+ static int client_mpvio_write_packet(struct st_plugin_vio *mpv,
+ const uchar *pkt, int pkt_len)
+ {
+ int res;
+ MCPVIO_EXT *mpvio= (MCPVIO_EXT*)mpv;
