Adjust design2: add preferred, and use true global with lock - #6
Conversation
… give error. If redirect is on, last msg not available, abort conn. If redirect conn fail, abort whole connect. Rename option enabled to enableRedirect
…'s comment, enhance document
| } | ||
|
|
||
| $lastError = error_get_last()["message"]; | ||
| if (strpos($lastError, "mysqlnd_azure.enableRedirect is on, but SSL option is not set. Redirection is only possible with SSL.") !== false) |
There was a problem hiding this comment.
Update connection string to: "mysqlnd_azure.enableRedirect is on, but SSL is not enabled on the server. Redirection is only possible with SSL."
There was a problem hiding this comment.
Hi Andrea,
The problem is different. It does not require ssl enabled (I think you mean enforced?) on server side, but need the connection is go with ssl, there need to set SSL option in connection string.
I've adjusted the message to "mysqlnd_azure.enableRedirect is on, but SSL option is not set in connection string. Redirection is only possible with SSL.", please check whether it is ok.
Christoph M. Becker (cmb69)
left a comment
Christoph M. Becker (cmb69)
left a comment
There was a problem hiding this comment.
This looks generally fine to me. I left two comments regarding possible macro use.
| } else if ((ZSTR_LEN(new_value) == 2 && strcasecmp("on", ZSTR_VAL(new_value)) == 0) | ||
| || (ZSTR_LEN(new_value) == 3 && strcasecmp("yes", ZSTR_VAL(new_value)) == 0) | ||
| || (ZSTR_LEN(new_value) == 4 && strcasecmp("true", ZSTR_VAL(new_value)) == 0) | ||
| || (ZSTR_LEN(new_value) == 1 && strcasecmp("1", ZSTR_VAL(new_value)) == 0)) { |
There was a problem hiding this comment.
Since zend_ini_parse_bool() is indeed only available as of PHP 7.3.0 (I wasn't aware of that), this is the way to go. A possible improvement might be to encapsulate each comparison in a macro, to make the code more readable and easier to maintain. Could read something like STRING_EQUALS(new_value, "1") then.
There was a problem hiding this comment.
Hi Christoph, I've made the change and added it in another separate PR without cache change which has been finished. Thanks a lot for your suggestion.
| if(MYSQLND_AZURE_G(enableRedirect) == REDIRECT_ON) { | ||
| //When REDIRECT_ON, if there is no redirection information contained in the last_message, then redirection is not possible. In this case, abort the connection | ||
| conn->m->send_close(conn); | ||
| SET_CLIENT_ERROR(conn->error_info, -1, UNKNOWN_SQLSTATE, "Connection aborted because redirection is not enabled on the MySQL server or the network package doesn't meet meet redirection protocol."); |
There was a problem hiding this comment.
Perhaps it's better to use a constant macro instead of the hard-coded -1 here, and elsewhere in this file?
|
|
||
| typedef enum _mysqlnd_azure_redirect_mode { | ||
| REDIRECT_OFF = 0, /* completely disabled */ | ||
| REDIRECT_ON = 1, /* enabled with fallback */ |
There was a problem hiding this comment.
is the comment of 'enabled with fallback' for 'preferred'?
| if(MYSQLND_AZURE_G(enableRedirect) == REDIRECT_ON) { | ||
| //When REDIRECT_ON, if there is no redirection information contained in the last_message, then redirection is not possible. In this case, abort the connection | ||
| conn->m->send_close(conn); | ||
| SET_CLIENT_ERROR(conn->error_info, -1, UNKNOWN_SQLSTATE, "Connection aborted because redirection is not enabled on the MySQL server or the network package doesn't meet meet redirection protocol."); |
There was a problem hiding this comment.
There are double 'meet' words in the sentence.
|
|
||
| } else { | ||
|
|
||
| MYSQLND_AZURE_G(enableRedirect) = REDIRECT_OFF; |
There was a problem hiding this comment.
I am not familiar with API. Just curious when this code path will be hit? If customer wants to set it 'on' to test his configuration, but set it by 'on1' by mistake, will the option changed to unexpected 'off' automatically?
There was a problem hiding this comment.
Yes, but that's customary for PHP.
5 participants
Rename option mysqlnd_azure.enabled to mysqlnd_azure.enableRedirect, and add a new option value "preferred".
When enableRedirect is "preferred", it will use redirection if possible. If connection does not use SSL, or server does not support redirection, or redirected connection fails to connect for any non-fatal reason while the proxy connection is still a valid one, it will fallback to the first proxy connection.
If enableRedirect is with value "on", SSL is off, no connection will be made, return error "mysqlnd_azure.enableRedirect is on, but SSL option is not set. Redirection is only possible with SSL."
If enableRedirect is with value "on", but on server side redirection is not supported, abort the first connection and return error "Abort the connection because MySQL server does not enable redirection or network package doesn't meet redirection protocol."
If enableRedirect is with value "on" and server supports redirection, but the redirected connection failed for any reason, also abort the first proxy connection. Return the error of the redirected connection.
The cache implementation has been changed from module global to true global to improve share range for multi-thread scenario.