maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Holyfoot!

On Sep 21, holyfoot@xxxxxxxxxxxx wrote:
> revision-id: d3f10e30ec39bccf1172c7b2eb159f40e412bdfd (mariadb-10.0.21-14-gd3f10e3)
> parent(s): abd31ca2b6cde024772fcac0f4953219d76c6344
> committer: Alexey Botchkov
> timestamp: 2015-09-21 14:06:15 +0500
> message:
> 
> MDEV-8431 Feedback plugin needs an option for http proxy.
> 'feedback_proxy_server' system variable added to specify the
> proxy server. Not a dynamic one.

How did you test this feature?

> diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc
> index f644bd5..f76c52a 100644
> --- a/plugin/feedback/feedback.cc
> +++ b/plugin/feedback/feedback.cc
> @@ -250,6 +251,9 @@ static int init(void *p)
>  
>    prepare_linux_info();
>  
> +  proxy_url= proxy_server ?
> +               Url::create(proxy_server, strlen(proxy_server)) : 0;

Some diagnostics would be nice here.
At least "feedback: invalid proxy url: '%.*s'".
See how ivalid feedback urls are reported.

> +
>    url_count= 0;
>    if (*url)
>    {
> @@ -347,6 +354,9 @@ static MYSQL_SYSVAR_ULONG(send_timeout, send_timeout, PLUGIN_VAR_RQCMDARG,
>  static MYSQL_SYSVAR_ULONG(send_retry_wait, send_retry_wait, PLUGIN_VAR_RQCMDARG,
>         "Wait this many seconds before retrying a failed send.",
>         NULL, NULL, 60, 1, 60*60*24, 1);
> +static MYSQL_SYSVAR_STR(proxy_server, proxy_server,
> +                        PLUGIN_VAR_READONLY | PLUGIN_VAR_RQCMDARG,
> +       "Proxy server address.", NULL, NULL,0);

Mention that it must be an url. "Proxy server URL" or
"Proxy server address, in the correct URI format", or something.

>  static struct st_mysql_sys_var* settings[] = {
>    MYSQL_SYSVAR(server_uid),
> diff --git a/plugin/feedback/url_http.cc b/plugin/feedback/url_http.cc
> index f214f7a..eade67c 100644
> --- a/plugin/feedback/url_http.cc
> +++ b/plugin/feedback/url_http.cc
> @@ -148,9 +148,12 @@ int Url_http::send(const char* data, size_t data_length)
>    my_socket fd= INVALID_SOCKET;
>    char buf[1024];
>    uint len= 0;
> +  Url_http *proxy_loc= (Url_http *) proxy_url;
>  
>    addrinfo *addrs, *addr, filter= {0, AF_UNSPEC, SOCK_STREAM, 6, 0, 0, 0, 0};
> -  int res= getaddrinfo(host.str, port.str, &filter, &addrs);
> +  int res= proxy_url ? 
> +    getaddrinfo(proxy_loc->host.str, proxy_loc->port.str, &filter, &addrs) :
> +    getaddrinfo(host.str, port.str, &filter, &addrs);
>  

First, you don't know whether proxy_url is Url_http. It might be easier
to have an option --feedback-http-proxy=host:port (not the url). It also
solves the potential future confusion if we'll have more than one
transport.

Second, I don't think proxies work this way at all, for a http proxy,
you need to specify the host in th GET request, like

  POST http://foo.bar/bla/bla

So, I think, a better implementation would be to take proxy into account
not in Url_http::send(), but already in http_create(). And create
Url_http objects for feedback url with proxy's host and port, and with
the appropriately adjusted path.

Regards,
Sergei