From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.8 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 1CEC2211B6 for ; Sat, 1 Dec 2018 13:31:27 +0000 (UTC) From: Eric Wong To: kcar-public@bogomips.org Subject: [PATCH 03/11] favor bitfields instead flags + macros Date: Sat, 1 Dec 2018 13:31:17 +0000 Message-Id: <20181201133125.5524-4-e@80x24.org> In-Reply-To: <20181201133125.5524-1-e@80x24.org> References: <20181201133125.5524-1-e@80x24.org> List-Id: 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