From: Eric Wong <e@80x24.org>
To: kcar-public@bogomips.org
Subject: [PATCH 03/11] favor bitfields instead flags + macros
Date: Sat, 1 Dec 2018 13:31:17 +0000 [thread overview]
Message-ID: <20181201133125.5524-4-e@80x24.org> (raw)
In-Reply-To: <20181201133125.5524-1-e@80x24.org>
This (IMHO) improves readability of code in the face of future
changes. At least on my system(*), binary size is even reduced:
text data bss dec hex filename
59233 1088 136 60457 ec29 before.so
58945 1088 136 60169 eb09 after.so
(*) x86-64 Debian gcc 4.9.2-10
---
ext/kcar/kcar.rl | 87 +++++++++++++++++++++------------------------
test/test_parser.rb | 1 +
2 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/ext/kcar/kcar.rl b/ext/kcar/kcar.rl
index 033e6ea..c0e8bbc 100644
--- a/ext/kcar/kcar.rl
+++ b/ext/kcar/kcar.rl
@@ -49,19 +49,17 @@ DEF_MAX_LENGTH(FRAGMENT, 1024); /* just in case (stolen from Mongrel) */
DEF_MAX_LENGTH(REQUEST_PATH, 4096); /* common PATH_MAX on modern systems */
DEF_MAX_LENGTH(QUERY_STRING, (1024 * 10));
-
-#define UH_FL_CHUNKED 0x1
-#define UH_FL_HASBODY 0x2
-#define UH_FL_INBODY 0x4
-#define UH_FL_HASTRAILER 0x8
-#define UH_FL_INTRAILER 0x10
-#define UH_FL_INCHUNK 0x20
-#define UH_FL_KEEPALIVE 0x40
-#define UH_FL_HASHEADER 0x80
-
struct http_parser {
int cs; /* Ragel internal state */
- unsigned int flags;
+ unsigned int chunked:1;
+ unsigned int has_body:1;
+ unsigned int in_body:1;
+ unsigned int has_trailer:1;
+ unsigned int in_trailer:1;
+ unsigned int in_chunk:1;
+ unsigned int persistent:1;
+ unsigned int has_header:1;
+ unsigned int padding:24;
unsigned int mark;
unsigned int offset;
union { /* these 2 fields don't nest */
@@ -99,11 +97,6 @@ static unsigned int ulong2uint(unsigned long n)
#define STR_NEW(M,FPC) rb_str_new(PTR_TO(M), LEN(M, FPC))
#define STRIPPED_STR_NEW(M,FPC) stripped_str_new(PTR_TO(M), LEN(M, FPC))
-#define HP_FL_TEST(hp,fl) ((hp)->flags & (UH_FL_##fl))
-#define HP_FL_SET(hp,fl) ((hp)->flags |= (UH_FL_##fl))
-#define HP_FL_UNSET(hp,fl) ((hp)->flags &= ~(UH_FL_##fl))
-#define HP_FL_ALL(hp,fl) (HP_FL_TEST(hp, fl) == (UH_FL_##fl))
-
/* Downcases a single ASCII character. Locale-agnostic. */
static void downcase_char(char *c)
{
@@ -143,7 +136,7 @@ static VALUE stripped_str_new(const char *str, long len)
static void finalize_header(struct http_parser *hp)
{
- if ((HP_FL_TEST(hp, HASTRAILER) && ! HP_FL_TEST(hp, CHUNKED)))
+ if (hp->has_trailer && !hp->chunked)
rb_raise(eParserError, "trailer but not chunked");
}
@@ -157,17 +150,17 @@ static void hp_keepalive_connection(struct http_parser *hp, VALUE val)
/* REQUEST_METHOD is always set before any headers */
if (STR_CSTR_CASE_EQ(val, "keep-alive")) {
/* basically have HTTP/1.0 masquerade as HTTP/1.1+ */
- HP_FL_SET(hp, KEEPALIVE);
+ hp->persistent = 1;
} else if (STR_CSTR_CASE_EQ(val, "close")) {
/*
* it doesn't matter what HTTP version or request method we have,
* if a server says "Connection: close", we disable keepalive
*/
- HP_FL_UNSET(hp, KEEPALIVE);
+ hp->persistent = 0;
} else {
/*
* server could've sent anything, ignore it for now. Maybe
- * "HP_FL_UNSET(hp, KEEPALIVE);" just in case?
+ * "hp->persistent = 0;" just in case?
* Raising an exception might be too mean...
*/
}
@@ -225,7 +218,7 @@ http_version(struct http_parser *hp, const char *ptr, size_t len)
{
if (CONST_MEM_EQ("HTTP/1.1", ptr, len)) {
/* HTTP/1.1 implies keepalive unless "Connection: close" is set */
- HP_FL_SET(hp, KEEPALIVE);
+ hp->persistent = 1;
}
}
@@ -243,12 +236,12 @@ status_phrase(struct http_parser *hp, VALUE hdr, const char *ptr, size_t len)
rb_raise(eParserError, "invalid status: %s", RSTRING_PTR(hp->status));
if ( !((nr >= 100 && nr <= 199) || nr == 204 || nr == 304) )
- HP_FL_SET(hp, HASBODY);
+ hp->has_body = 1;
}
static inline void invalid_if_trailer(struct http_parser *hp)
{
- if (HP_FL_TEST(hp, INTRAILER))
+ if (hp->in_trailer)
rb_raise(eParserError, "invalid Trailer");
}
@@ -299,7 +292,7 @@ static void write_value(VALUE hdr, struct http_parser *hp,
const char *vptr;
size_t vlen;
- HP_FL_SET(hp, HASHEADER);
+ hp->has_header = 1;
/* Rack does not like Status headers, so we never send them */
if (CSTR_CASE_EQ(fptr, flen, "status")) {
@@ -323,9 +316,9 @@ static void write_value(VALUE hdr, struct http_parser *hp,
if (STR_CSTR_CASE_EQ(f, "connection")) {
hp_keepalive_connection(hp, v);
} else if (STR_CSTR_CASE_EQ(f, "content-length")) {
- if (! HP_FL_TEST(hp, HASBODY))
+ if (!hp->has_body)
rb_raise(eParserError, "Content-Length with no body expected");
- if (HP_FL_TEST(hp, CHUNKED))
+ if (hp->chunked)
rb_raise(eParserError,
"Content-Length when chunked Transfer-Encoding is set");
hp->len.content = parse_length(vptr, vlen);
@@ -336,7 +329,7 @@ static void write_value(VALUE hdr, struct http_parser *hp,
invalid_if_trailer(hp);
} else if (STR_CSTR_CASE_EQ(f, "transfer-encoding")) {
if (STR_CSTR_CASE_EQ(v, "chunked")) {
- if (! HP_FL_TEST(hp, HASBODY))
+ if (!hp->has_body)
rb_raise(eParserError,
"chunked Transfer-Encoding with no body expected");
if (hp->len.content >= 0)
@@ -344,13 +337,13 @@ static void write_value(VALUE hdr, struct http_parser *hp,
"chunked Transfer-Encoding when Content-Length is set");
hp->len.chunk = 0;
- HP_FL_SET(hp, CHUNKED);
+ hp->chunked = 1;
}
invalid_if_trailer(hp);
} else if (STR_CSTR_CASE_EQ(f, "trailer")) {
- if (! HP_FL_TEST(hp, HASBODY))
+ if (!hp->has_body)
rb_raise(eParserError, "trailer with no body");
- HP_FL_SET(hp, HASTRAILER);
+ hp->has_trailer = 1;
invalid_if_trailer(hp);
}
@@ -425,7 +418,7 @@ static void write_value(VALUE hdr, struct http_parser *hp,
finalize_header(hp);
cs = http_parser_first_final;
- if (HP_FL_TEST(hp, CHUNKED))
+ if (hp->chunked)
cs = http_parser_en_ChunkedBody;
/*
@@ -441,7 +434,7 @@ static void write_value(VALUE hdr, struct http_parser *hp,
}
action end_chunked_body {
- HP_FL_SET(hp, INTRAILER);
+ hp->in_trailer = 1;
cs = http_parser_en_Trailers;
++p;
assert(p <= pe && "buffer overflow after chunked body");
@@ -457,7 +450,7 @@ static void write_value(VALUE hdr, struct http_parser *hp,
p += nr;
assert(hp->len.chunk >= 0 && "negative chunk length");
if ((size_t)hp->len.chunk > REMAINING) {
- HP_FL_SET(hp, INCHUNK);
+ hp->in_chunk = 1;
goto post_exec;
} else {
fhold;
@@ -501,8 +494,8 @@ static void http_parser_execute(struct http_parser *hp,
assert((void *)(pe - p) == (void *)(len - off) &&
"pointers aren't same distance");
- if (HP_FL_TEST(hp, INCHUNK)) {
- HP_FL_UNSET(hp, INCHUNK);
+ if (hp->in_chunk) {
+ hp->in_chunk = 0;
goto skip_chunk_data_hack;
}
%% write exec;
@@ -603,7 +596,7 @@ static VALUE body_bytes_left(VALUE self)
{
struct http_parser *hp = data_get(self);
- if (HP_FL_TEST(hp, CHUNKED))
+ if (hp->chunked)
return Qnil;
if (hp->len.content >= 0)
return OFFT2NUM(hp->len.content);
@@ -623,7 +616,7 @@ static VALUE body_bytes_left_set(VALUE self, VALUE bytes)
{
struct http_parser *hp = data_get(self);
- if (HP_FL_TEST(hp, CHUNKED))
+ if (hp->chunked)
rb_raise(rb_eRuntimeError, "body_bytes_left= is not for chunked bodies");
hp->len.content = NUM2OFFT(bytes);
return bytes;
@@ -640,7 +633,7 @@ static VALUE chunked(VALUE self)
{
struct http_parser *hp = data_get(self);
- return HP_FL_TEST(hp, CHUNKED) ? Qtrue : Qfalse;
+ return hp->chunked ? Qtrue : Qfalse;
}
static void check_buffer_size(long dlen)
@@ -674,7 +667,7 @@ static VALUE headers(VALUE self, VALUE hdr, VALUE data)
hp->cs == http_parser_en_ChunkedBody) {
advance_str(data, hp->offset + 1);
hp->offset = 0;
- if (HP_FL_TEST(hp, INTRAILER))
+ if (hp->in_trailer)
return hdr;
else
return rb_ary_new3(2, hp->status, hdr);
@@ -688,7 +681,7 @@ static VALUE headers(VALUE self, VALUE hdr, VALUE data)
static int chunked_eof(struct http_parser *hp)
{
- return ((hp->cs == http_parser_first_final) || HP_FL_TEST(hp, INTRAILER));
+ return ((hp->cs == http_parser_first_final) || hp->in_trailer);
}
/**
@@ -702,13 +695,13 @@ static VALUE body_eof(VALUE self)
{
struct http_parser *hp = data_get(self);
- if (!HP_FL_TEST(hp, HASHEADER) && HP_FL_ALL(hp, KEEPALIVE))
+ if (!hp->has_header && hp->persistent)
return Qtrue;
- if (HP_FL_TEST(hp, CHUNKED))
+ if (hp->chunked)
return chunked_eof(hp) ? Qtrue : Qfalse;
- if (! HP_FL_TEST(hp, HASBODY))
+ if (!hp->has_body)
return Qtrue;
return hp->len.content == 0 ? Qtrue : Qfalse;
@@ -730,9 +723,9 @@ static VALUE keepalive(VALUE self)
{
struct http_parser *hp = data_get(self);
- if (HP_FL_ALL(hp, KEEPALIVE)) {
- if (HP_FL_TEST(hp, HASHEADER) && HP_FL_TEST(hp, HASBODY) ) {
- if (HP_FL_TEST(hp, CHUNKED) || (hp->len.content >= 0))
+ if (hp->persistent) {
+ if (hp->has_header && hp->has_body) {
+ if (hp->chunked || (hp->len.content >= 0))
return Qtrue;
/* unknown Content-Length and not chunked, we must assume close */
@@ -774,7 +767,7 @@ static VALUE filter_body(VALUE self, VALUE buf, VALUE data)
rb_str_resize(buf, dlen); /* we can never copy more than dlen bytes */
OBJ_TAINT(buf); /* keep weirdo $SAFE users happy */
- if (!HP_FL_TEST(hp, CHUNKED))
+ if (!hp->chunked)
rb_raise(rb_eRuntimeError, "filter_body is only for chunked bodies");
if (!chunked_eof(hp)) {
diff --git a/test/test_parser.rb b/test/test_parser.rb
index 3768808..84e609d 100644
--- a/test/test_parser.rb
+++ b/test/test_parser.rb
@@ -300,6 +300,7 @@ class TestParser < Test::Unit::TestCase
require 'objspace'
n = ObjectSpace.memsize_of(@hp)
assert_kind_of Integer, n
+ warn "memsize: #{n}\n" if $DEBUG
rescue LoadError
warn 'ObjectSpace not available'
end
next prev parent reply other threads:[~2018-12-01 13:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-01 13:31 [PATCH v2] request parsing bits Eric Wong
2018-12-01 13:31 ` [PATCH 01/11] introduce new str_new_dd_freeze internal function Eric Wong
2018-12-01 13:31 ` [PATCH 02/11] begin implementing request parsing Eric Wong
2018-12-01 13:31 ` Eric Wong [this message]
2018-12-01 13:31 ` [PATCH 04/11] implement request parsing with tests Eric Wong
2018-12-01 13:31 ` [PATCH 05/11] pkg.mk: enable warnings by default for tests Eric Wong
2018-12-01 13:31 ` [PATCH 06/11] filter_body: rename variables to be like memcpy(3) Eric Wong
2018-12-01 13:31 ` [PATCH 07/11] flesh out filter_body for request parsing Eric Wong
2018-12-01 13:31 ` [PATCH 08/11] do not assume SERVER_PORT Eric Wong
2018-12-01 13:31 ` [PATCH 09/11] do not set "HTTP/0.9" for pre-1.0 requests Eric Wong
2018-12-01 13:31 ` [PATCH 10/11] always set non-negative Content-Length for requests Eric Wong
2018-12-01 13:31 ` [PATCH 11/11] avoid String#-@ call on request parsing under Ruby 2.6 Eric Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://yhbt.net/kcar/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181201133125.5524-4-e@80x24.org \
--to=e@80x24.org \
--cc=kcar-public@bogomips.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhbt.net/kcar.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).