Re: [patches] Re[2]: [PATCH] add support for http+unix protocol

[Konstantin Belyavskiy]

Revert back version with malloc

diff --git a/src/httpc.c b/src/httpc.c
index 84ea67aba..b3849699b 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -32,10 +32,36 @@
#include "httpc.h"

#include <assert.h>
+#include <ctype.h>
#include <curl/curl.h>

#include "fiber.h"

+static const char *HTTP_LOCALHOST = "http://localhost";
+static const char *HTTP_UNIX_PREFIX = "http+unix://";
+
+/**
+ * Function to decode encoded symbols in http url.
+ */
+static int
+urldecode(const char *s, const char *end, char *dec)
+{
+ char *o;
+ int c;
+
+ for (o = dec; s != end; o++) {
+ c = *s++;
+ if (c == '+') c = ' ';
+ else if (c == '%' && (!isxdigit(*s++) ||
+ !isxdigit(*s++) ||
+ !sscanf(s - 2, "%2x", &c)))
+ return -1;
+ *o = c;
+ }
+ *o = '\0';
+ return o - dec;
+}
+
/**
* libcurl callback for CURLOPT_WRITEFUNCTION
* @see https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html
@@ -134,7 +160,29 @@ httpc_request_new(struct httpc_env *env, const char *method,
curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
}

- curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
+ const int prefix_len = strlen(HTTP_UNIX_PREFIX);
+ if (strncmp(url, HTTP_UNIX_PREFIX, prefix_len) == 0) {
+ const int url_len = strlen(url);
+ char *http_url = (char *) malloc(strlen(HTTP_LOCALHOST) +
+ url_len - prefix_len + 1);
+
+ const char *socket_path_start = url + prefix_len;
+ const char *socket_path_end = strchr(socket_path_start, '/');
+ if (socket_path_end == NULL)
+ socket_path_end = url + url_len;
+
+ char *socket_path = (char *) malloc(socket_path_end - socket_path_start + 1);
+ urldecode(socket_path_start, socket_path_end, socket_path);
+ sprintf(http_url, "%s%s", HTTP_LOCALHOST, socket_path_end);
+
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_UNIX_SOCKET_PATH, socket_path);
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, http_url);
+
+ free(http_url);
+ free(socket_path);
+ } else {
+ curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
+ }

curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);


>Пятница, 26 января 2018, 16:50 +03:00 от Konstantin Belyavskiy <k.belyavskiy at tarantool.org>:
>
>
>src/httpc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>1 file changed, 52 insertions(+), 1 deletion(-)
>diff --git a/src/httpc.c b/src/httpc.c
>index 84ea67aba..885ce1b98 100644
>--- a/src/httpc.c
>+++ b/src/httpc.c
>@@ -32,10 +32,36 @@
>#include "httpc.h"
>
>#include <assert.h>
>+#include <ctype.h>
>#include <curl/curl.h>
>
>#include "fiber.h"
>
>+static const char *HTTP_LOCALHOST = "http://localhost";
>+static const char *HTTP_UNIX_PREFIX = "http+unix://";
>+
>+/**
>+ * Function to decode encoded symbols in http url.
>+ */
>+static int
>+urldecode(const char *s, const char *end, char *dec)
>+{
>+ char *o;
>+ int c;
>+
>+ for (o = dec; s != end; o++) {
>+ c = *s++;
>+ if (c == '+') c = ' ';
>+ else if (c == '%' && (!isxdigit(*s++) ||
>+ !isxdigit(*s++) ||
>+ !sscanf(s - 2, "%2x", &c)))
>+ return -1;
>+ *o = c;
>+ }
>+ *o = '\0';
>+ return o - dec;
>+}
>+
>/**
>* libcurl callback for CURLOPT_WRITEFUNCTION
>* @see  https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html
>@@ -134,7 +160,32 @@ httpc_request_new(struct httpc_env *env, const char *method,
>curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
>}
>
>- curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
>+ const int prefix_len = strlen(HTTP_UNIX_PREFIX);
>+ if (strncmp(url, HTTP_UNIX_PREFIX, prefix_len) == 0) {
>+ const int url_len = strlen(url);
>+ struct region region;
>+ struct slab_cache cache;
>+ region_create(&region, &cache);
>+ char *http_url = (char *) region_alloc(&region,
>+ strlen(HTTP_LOCALHOST) + url_len - prefix_len + 1);
>+
>+ const char *socket_path_start = url + prefix_len;
>+ const char *socket_path_end = strchr(socket_path_start, '/');
>+ if (socket_path_end == NULL)
>+ socket_path_end = url + url_len;
>+
>+ char *socket_path = (char *) region_alloc(&region,
>+ socket_path_end - socket_path_start + 1);
>+ urldecode(socket_path_start, socket_path_end, socket_path);
>+ sprintf(http_url, "%s%s", HTTP_LOCALHOST, socket_path_end);
>+
>+ curl_easy_setopt(req->curl_request.easy, CURLOPT_UNIX_SOCKET_PATH, socket_path);
>+ curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, http_url);
>+
>+ region_free(&region);
>+ } else {
>+ curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
>+ }
>
>curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
>curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
>
> 
>
>
>>Пятница, 26 января 2018, 14:19 +03:00 от Vladimir Davydov < vdavydov.dev at gmail.com >:
>>
>>Style: Would be nice to add 'httpc: ' to the commit message subject:
>>
>>  httpc: add support for unix protocol
>>
>>On Fri, Jan 26, 2018 at 11:51:14AM +0300, Konstantin Belyavskiy wrote:
>>> The http+unix:// is http protocol over unix domain sockets.
>>> implementation based on original racktear proposal
>>> decided to support only http+unix format
>>> decided not to use our uri parser
>>
>>Style: I don't think it's worth enumerating what we decided NOT to do.
>>
>>> 
>>> closes #3040
>>
>>Style: Should start with a capital letter.
>>
>>> ---
>>> branch: gh-3040-unix-domain-sockets-orig-fix-formatting
>>>  src/httpc.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  src/httpc.h |  1 +
>>
>>Please add a test.
>>
>>>  2 files changed, 49 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/httpc.c b/src/httpc.c
>>> index 84ea67aba..d5ad1164b 100644
>>> --- a/src/httpc.c
>>> +++ b/src/httpc.c
>>> @@ -36,6 +36,31 @@
>>> 
>>>  #include "fiber.h"
>>> 
>>> +static const char *HTTP_LOCALHOST = "http://localhost";
>>> +static const char *HTTP_UNIX_PREFIX = "http+unix://";
>>> +
>>> +/**
>>> + * function to decode encoded symbols in http url
>>
>>Style: Start sentences with a capital letter and end with a dot.
>>
>>> + */
>>> +static int
>>> +urldecode(const char *s, const char *end, char *dec)
>>> +{
>>> +	char *o;
>>> +	int c;
>>> +
>>> +	for (o = dec; s <= end; o++) {
>>
>>In our code, 'end' usually points to the entry following the last one in
>>a set (just like in STL) and this is actually true in your case. So this
>>should be:
>>
>>  for (o = dec; s != end; o++)
>>
>>> +		c = *s++;
>>> +		if (c == '+') c = ' ';
>>> +		else if (c == '%' && (!isxdigit(*s++) ||
>>> +				      !isxdigit(*s++) ||
>>> +				      !sscanf(s - 2, "%2x", &c)))
>>> +			return -1;
>>> +		*o = c;
>>> +	}
>>
>>I think this function should set the terminating nul.
>>
>>> +
>>> +	return o - dec;
>>> +}
>>> +
>>>  /**
>>>   * libcurl callback for CURLOPT_WRITEFUNCTION
>>>   * @see  https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html
>>> @@ -134,7 +159,29 @@ httpc_request_new(struct httpc_env *env, const char *method,
>>>  		curl_easy_setopt(req->curl_request.easy, CURLOPT_CUSTOMREQUEST, method);
>>>  	}
>>> 
>>> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
>>> +	const int prefix_len = strlen(HTTP_UNIX_PREFIX);
>>> +	if (strncmp(url, HTTP_UNIX_PREFIX, prefix_len) == 0) {
>>> +		const int url_len = strlen(url);
>>> +		char *http_url = (char *) malloc(strlen(HTTP_LOCALHOST) +
>>> +						 url_len - prefix_len + 1);
>>
>>Please consider using the region allocator instead of malloc if
>>possible - it is much more efficient in terms of performance.
>>And don't forget to handle allocation failures.
>>
>>> +
>>> +		const char *socket_path_start = url + prefix_len;
>>> +		const char *socket_path_end = strchr(socket_path_start, '/');
>>> +		if (socket_path_end == NULL)
>>> +			socket_path_end = url + url_len;
>>> +
>>> +		char *socket_path = (char *) malloc(socket_path_end - socket_path_start + 1);
>>> +		urldecode(socket_path_start, socket_path_end, socket_path);
>>> +		sprintf(http_url, "http://localhost%s", socket_path_end);
>>
>>Please reuse HTTP_LOCALHOST here:
>>
>>  sprintf(http_url, HTTP_LOCALHOST "%s", socket_path_end);
>>
>>> +
>>> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_UNIX_SOCKET_PATH, socket_path);
>>> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, http_url);
>>> +
>>> +		free(http_url);
>>> +		free(socket_path);
>>> +	} else {
>>> +		curl_easy_setopt(req->curl_request.easy, CURLOPT_URL, url);
>>> +	}
>>> 
>>>  	curl_easy_setopt(req->curl_request.easy, CURLOPT_FOLLOWLOCATION, 1);
>>>  	curl_easy_setopt(req->curl_request.easy, CURLOPT_SSL_VERIFYPEER, 1);
>>> diff --git a/src/httpc.h b/src/httpc.h
>>> index 3358fd914..8c3e75f75 100644
>>> --- a/src/httpc.h
>>> +++ b/src/httpc.h
>>> @@ -34,6 +34,7 @@
>>>  #include <small/region.h>
>>>  #include <small/mempool.h>
>>>  #include <tarantool_ev.h>
>>> +#include <ctype.h>
>>
>>Please avoid including in headers - this slows down compilation.
>>Include in C files if possible.
>
>
>С уважением,
>Konstantin Belyavskiy
>k.belyavskiy at tarantool.org


С уважением,
Konstantin Belyavskiy
k.belyavskiy at tarantool.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180126/04806779/attachment.html>