[PATCH gwhttpd 0/7] gwhttpd updates

[PATCH gwhttpd 0/7] gwhttpd updates

[Ammar Faizi]

Hi,

This series contains gwhttpd refactoring and updates. I am going
to add directory traversing support feature, but before that,
several cleanups need to get done. And here it is...

Summary below...

## Patch 1
Don't continue executing the next line if we fail to parse the
HTTP header, that's the wrong path, we should stop the client
early.

## Patch 2
This mlock is just optional, don't bother showing error.

## Patch 3
Let the caller close the connection, we need to have more decisions
in the caller rather than closing it directly.

## Patch 4
Print a log message that we are interrupted, this makes the app user
friendly because the user can explicitly see from the output that the
app is interrupted.

## Patch 5
The HTTP header parser was prone and doesn't handle many edge cases.
This is a full refactor of HTTP header parser. While in there, add
more HTTP method supports.

## Patch 6
When we fail to send() due to EAGAIN, we retry the send(). However,
if we are spinning on this retry for so long, this will eat CPU cycle
and slow down the entire application. We should stop retrying at
some point. Add a loop counter and return -ENETDOWN if we are failing
too many times in the send() retry loop cycle.

## Patch 7
A patch for Makefile. Add a command to clean the binary file.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (7):
  gwhttpd: Do an early return when parse_http_header fails
  gwhttpd: Don't print any error when mlock fails
  gwhttpd: Replace `send_error_and_close` with `send_http_error()`
  gwhttpd: Add log in the interrupt handler
  gwhttpd: Refactor HTTP header parser
  gwhttpd: Avoid endless busy spinning on `send()`
  Makefile: Add "make clean" command

 Makefile    |   3 +
 gwhttpd.cpp | 220 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 161 insertions(+), 62 deletions(-)

base-commit: 944b77d9aaac4a88729da6ec2f76447a9c3562ab
-- 
Ammar Faizi

[PATCH gwhttpd 1/7] gwhttpd: Do an early return when parse_http_header fails

[Ammar Faizi]

Don't continue executing the next line if we fail to parse the
HTTP header, that's the wrong path, we should stop the client
early.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index 0f4261d..33b58a1 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -498,8 +498,10 @@ static int _handle_client(struct client_sess *sess, struct server_state *state)
 	sess->buf[sess->buf_size] = '\0';
 	if (!sess->got_http_header) {
 		ret = parse_http_header(sess);
-		if (ret)
+		if (ret) {
 			send_error_and_close(400, sess, state);
+			return 0;
+		}
 		if (!sess->got_http_header)
 			return 0;
 	}
-- 
Ammar Faizi

[PATCH gwhttpd 2/7] gwhttpd: Don't print any error when mlock fails

[Ammar Faizi]

This mlock is just optional, don't bother showing error.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index 33b58a1..b01d5f1 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -647,9 +647,7 @@ int main(int argc, char *argv[])
 		return ret;
 	}
 
-	ret = mlock(state, sizeof(*state));
-	if (ret < 0)
-		perror("mlock");
+	mlock(state, sizeof(*state));
 #endif
 
 	g_state = state;
-- 
Ammar Faizi

[PATCH gwhttpd 3/7] gwhttpd: Replace `send_error_and_close` with `send_http_error()`

[Ammar Faizi]

Let the caller close the connection, we need to have more decisions
in the caller rather than closing it directly.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index b01d5f1..afe4c3a 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -369,22 +369,17 @@ repeat:
 	return ret;
 }
 
-static void send_error_and_close(int code, struct client_sess *sess,
-				 struct server_state *state)
+static void send_http_error(int code, struct client_sess *sess)
 {
 	char buf[128];
-	ssize_t ret;
 	int tmp;
 
 	tmp = snprintf(buf, sizeof(buf),
 			"HTTP/1.1 %d\r\n"
 			"Content-Type: text/plain\r\n\r\n"
-			"Error %d",
+			"HTTP Error %d",
 			code, code);
-
-	ret = send_to_client(sess, buf, (size_t)tmp);
-	if (ret < 0)
-		close_sess(sess, state);
+	send_to_client(sess, buf, (size_t)tmp);
 }
 
 #define HTTP_200_HTML "HTTP/1.1 200\r\nContent-Type: text/html\r\n\r\n"
@@ -434,7 +429,8 @@ static int handle_route_get(struct client_sess *sess,
 	if (!strcmp(uri, "/hello"))
 		return route_show_hello(sess, state);
 
-	send_error_and_close(404, sess, state);
+	send_http_error(404, sess);
+	close_sess(sess, state);
 	return 0;
 }
 
@@ -465,7 +461,8 @@ static int handle_route_post(struct client_sess *sess,
 	if (!strcmp(uri, "/echo"))
 		return route_show_echo(sess, state);
 
-	send_error_and_close(404, sess, state);
+	send_http_error(404, sess);
+	close_sess(sess, state);
 	return 0;
 }
 
@@ -481,14 +478,12 @@ static int handle_route(struct client_sess *sess, struct server_state *state)
 		ret = handle_route_post(sess, state);
 		break;
 	default:
-		send_error_and_close(405, sess, state);
+		send_http_error(405, sess);
+		close_sess(sess, state);
 		return 0;
 	}
 
-	if (ret)
-		close_sess(sess, state);
-
-	return 0;
+	return ret;
 }
 
 static int _handle_client(struct client_sess *sess, struct server_state *state)
@@ -499,14 +494,20 @@ static int _handle_client(struct client_sess *sess, struct server_state *state)
 	if (!sess->got_http_header) {
 		ret = parse_http_header(sess);
 		if (ret) {
-			send_error_and_close(400, sess, state);
+			send_http_error(400, sess);
 			return 0;
 		}
 		if (!sess->got_http_header)
 			return 0;
 	}
 
-	return handle_route(sess, state);
+	ret = handle_route(sess, state);
+	if (ret) {
+		close_sess(sess, state);
+		if (likely(ret == -EBADMSG || ret == -ENETDOWN))
+			ret = 0;
+	}
+	return ret;
 }
 
 static int handle_client(struct client_sess *sess, struct server_state *state)
-- 
Ammar Faizi

[PATCH gwhttpd 4/7] gwhttpd: Add log in the interrupt handler

[Ammar Faizi]

Print a log message that we are interrupted, this makes the app user
friendly because the user can explicitly see from the output that the
app is interrupted.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index afe4c3a..ce280de 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -67,6 +67,8 @@ static struct server_state *g_state;
 
 static void interrupt_handler(int sig)
 {
+	putchar('\n');
+	printf("Got signal: %d\n", sig);
 	if (!g_state)
 		return;
 
-- 
Ammar Faizi

[PATCH gwhttpd 5/7] gwhttpd: Refactor HTTP header parser

[Ammar Faizi]

The HTTP header parser was prone and doesn't handle many edge cases.
This is a full refactor of HTTP header parser. While in there, add
more HTTP method supports.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 183 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 136 insertions(+), 47 deletions(-)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index ce280de..fbe219d 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -30,7 +30,10 @@
 
 enum http_method {
 	HTTP_GET,
-	HTTP_POST
+	HTTP_POST,
+	HTTP_PATCH,
+	HTTP_PUT,
+	HTTP_DELETE,
 };
 
 struct client_sess {
@@ -39,6 +42,7 @@ struct client_sess {
 	char			buf[4096];
 	enum http_method	method;
 	char			*uri;
+	char			*qs;
 	char			*http_ver;
 	char			*body;
 	bool			got_http_header;
@@ -305,48 +309,6 @@ static int handle_new_client(struct server_state *state)
 	return 0;
 }
 
-static int parse_http_header(struct client_sess *sess)
-{
-	char *tmp;
-
-	/*
-	 * CRLF = Carriage Return, Line Feed.
-	 */
-	tmp = strstr(sess->buf, "\r\n\r\n");
-	if (!tmp)
-		return 0;
-
-	sess->body = tmp + 4;
-	if (!strncmp(sess->buf, "GET", sizeof("GET") - 1)) {
-		sess->method = HTTP_GET;
-		sess->uri = &sess->buf[sizeof("GET")];
-	} else if (!strncmp(sess->buf, "POST", sizeof("POST") - 1)) {
-		sess->method = HTTP_POST;
-		sess->uri = &sess->buf[sizeof("POST")];
-	} else {
-		return -EBADMSG;
-	}
-
-	sess->http_ver = strstr(sess->uri, " ");
-	if (!sess->http_ver)
-		return -EBADMSG;
-
-	sess->http_ver[0] = '\0';
-	tmp = strstr(++sess->http_ver, "\r\n");
-	tmp[0] = '\0';
-	sess->got_http_header = true;
-	return 0;
-}
-
-static void close_sess(struct client_sess *sess, struct server_state *state)
-{
-	printf("Closing session " FCIP " (idx = %u)\n", FCIP_ARG(sess),
-		sess->idx);
-	delete_client(state->epl_fd, sess);
-	close(sess->fd);
-	put_sess_idx(sess->idx, state);
-}
-
 static ssize_t send_to_client(struct client_sess *sess, const char *buf,
 			      size_t len)
 {
@@ -384,6 +346,128 @@ static void send_http_error(int code, struct client_sess *sess)
 	send_to_client(sess, buf, (size_t)tmp);
 }
 
+static char *parse_http_method(char *buf, enum http_method *method_p)
+{
+	if (!strncmp("GET ", buf, sizeof("GET"))) {
+		*method_p = HTTP_GET;
+		return buf + sizeof("GET");
+	}
+
+	if (!strncmp("POST ", buf, sizeof("POST"))) {
+		*method_p = HTTP_POST;
+		return buf + sizeof("POST");
+	}
+
+	if (!strncmp("PUT ", buf, sizeof("PUT"))) {
+		*method_p = HTTP_PUT;
+		return buf + sizeof("PUT");
+	}
+
+	if (!strncmp("PATCH ", buf, sizeof("PATCH"))) {
+		*method_p = HTTP_PATCH;
+		return buf + sizeof("PATCH");
+	}
+
+	if (!strncmp("DELETE ", buf, sizeof("DELETE"))) {
+		*method_p = HTTP_DELETE;
+		return buf + sizeof("DELETE");
+	}
+
+	return NULL;
+}
+
+static char *parse_query_string(char *uri, char *end_uri)
+{
+	while (uri < end_uri) {
+		if (*uri++ != '?')
+			continue;
+		if (uri == end_uri)
+			/*
+			 * We got an empty query string:
+			 *  "http://somehwere.com/path?"
+			 */
+			return NULL;
+		return uri;
+	}
+	return NULL;
+}
+
+static int parse_http_header(struct client_sess *sess)
+{
+	char *buf = sess->buf;
+	char *end;
+	char *ret;
+
+	/*
+	 * Split the HTTP header and HTTP body.
+	 */
+	ret = strstr(buf, "\r\n\r\n");
+	if (!ret) {
+		/*
+		 * If we fail here, we may got a partial packet.
+		 * Don't fail here if still have enough buffer,
+		 * we will wait for the next recv() iteration.
+		 */
+		if (sess->buf_size >= sizeof(sess->buf) - 1)
+			goto bad_req;
+
+		return 0;
+	}
+	end = ret;
+
+	/*
+	 * The HTTP body is located right after "\r\n\r\n".
+	 */
+	sess->body = &ret[4];
+
+	ret = parse_http_method(buf, &sess->method);
+	if (unlikely(!ret))
+		goto bad_req;
+
+	/*
+	 * Now @ret is pointing to URI. For example:
+	 *
+	 *  "GET / HTTP/1.1"
+	 *       ^
+	 *     @ret
+	 */
+	sess->uri = ret;
+	ret = strstr(sess->uri, " ");
+	if (unlikely(!ret))
+		goto bad_req;
+
+	ret[0] = '\0';
+	if (unlikely(&ret[1] >= end))
+		goto bad_req;
+
+	sess->qs = parse_query_string(sess->uri, end);
+	ret = strstr(&ret[1], "HTTP/");
+	if (unlikely(!ret))
+		goto bad_req;
+
+	sess->http_ver = ret;
+	ret = strstr(sess->http_ver, "\r\n");
+	if (unlikely(!ret))
+		goto bad_req;
+
+	ret[0] = '\0';
+	sess->got_http_header = true;
+	return 0;
+
+bad_req:
+	send_http_error(400, sess);
+	return -EBADMSG;
+}
+
+static void close_sess(struct client_sess *sess, struct server_state *state)
+{
+	printf("Closing session " FCIP " (idx = %u)\n", FCIP_ARG(sess),
+		sess->idx);
+	delete_client(state->epl_fd, sess);
+	close(sess->fd);
+	put_sess_idx(sess->idx, state);
+}
+
 #define HTTP_200_HTML "HTTP/1.1 200\r\nContent-Type: text/html\r\n\r\n"
 #define HTTP_200_TEXT "HTTP/1.1 200\r\nContent-Type: text/plain\r\n\r\n"
 
@@ -495,15 +579,20 @@ static int _handle_client(struct client_sess *sess, struct server_state *state)
 	sess->buf[sess->buf_size] = '\0';
 	if (!sess->got_http_header) {
 		ret = parse_http_header(sess);
-		if (ret) {
-			send_http_error(400, sess);
-			return 0;
-		}
+		if (ret)
+			goto out;
 		if (!sess->got_http_header)
 			return 0;
 	}
 
+#if 0
+	printf("URI: %s\n", sess->uri);
+	printf("Query String: %s\n", sess->qs);
+	printf("HTTP version: %s\n", sess->http_ver);
+#endif
+
 	ret = handle_route(sess, state);
+out:
 	if (ret) {
 		close_sess(sess, state);
 		if (likely(ret == -EBADMSG || ret == -ENETDOWN))
-- 
Ammar Faizi

[PATCH gwhttpd 6/7] gwhttpd: Avoid endless busy spinning on `send()`

[Ammar Faizi]

When we fail to send() due to EAGAIN, we retry the send(). However,
if we are spinning on this retry for so long, this will eat CPU cycle
and slow down the entire application. We should stop retrying at
some point. Add a loop counter and return -ENETDOWN if we are failing
too many times in the send() retry loop cycle.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 gwhttpd.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gwhttpd.cpp b/gwhttpd.cpp
index fbe219d..73a1d84 100644
--- a/gwhttpd.cpp
+++ b/gwhttpd.cpp
@@ -312,10 +312,14 @@ static int handle_new_client(struct server_state *state)
 static ssize_t send_to_client(struct client_sess *sess, const char *buf,
 			      size_t len)
 {
+	constexpr uint32_t max_try = 10;
+	uint32_t try_count = 0;
 	ssize_t ret;
 	int tmp;
 
 repeat:
+	if (unlikely(try_count++ >= max_try))
+		return -ENETDOWN;
 	ret = send(sess->fd, buf, len, MSG_DONTWAIT);
 	if (unlikely(ret < 0)) {
 		tmp = errno;
-- 
Ammar Faizi

[PATCH gwhttpd 7/7] Makefile: Add "make clean" command

[Ammar Faizi]

Add a command to clean the binary file.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index 14522cb..28cc0c8 100644
--- a/Makefile
+++ b/Makefile
@@ -7,3 +7,6 @@ endif
 
 gwhttpd: gwhttpd.cpp
 	$(CXX) $(CXXFLAGS) -o $(@) $(^)
+
+clean:
+	rm -vf gwhttpd
-- 
Ammar Faizi