From 10886a31a2db39edf9f4cb3e13ad5abb941d8018 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 12 Feb 2022 21:46:15 +0100 Subject: [PATCH] * Fixed inefficient RESTCONF HTTP/1 read body by appending bufs instead of re-parsing * Fixed: http1 by mistake included in fcgi build * Test: restconf perf test uses restconf for initial setup instead of netconf --- apps/restconf/Makefile.in | 3 ++ apps/restconf/restconf_http1.c | 36 +++++++++++-------- apps/restconf/restconf_http1.h | 2 +- apps/restconf/restconf_native.c | 63 ++++++++++++++++++++++++++------- test/test_perf_restconf.sh | 28 +++++++-------- test/test_restconf_continue.sh | 1 + 6 files changed, 89 insertions(+), 44 deletions(-) diff --git a/apps/restconf/Makefile.in b/apps/restconf/Makefile.in index 965a8df3..d3c44a2e 100644 --- a/apps/restconf/Makefile.in +++ b/apps/restconf/Makefile.in @@ -114,7 +114,10 @@ APPSRC += restconf_stream_$(with_restconf).c endif # internal http/1 parser +YACCOBJS = +ifeq ($(with_restconf),native) YACCOBJS = lex.clixon_http1_parse.o clixon_http1_parse.tab.o +endif APPOBJ = $(APPSRC:.c=.o) $(YACCOBJS) diff --git a/apps/restconf/restconf_http1.c b/apps/restconf/restconf_http1.c index bb6a2bb4..b69e72c9 100644 --- a/apps/restconf/restconf_http1.c +++ b/apps/restconf/restconf_http1.c @@ -464,29 +464,35 @@ http1_check_expect(clicon_handle h, /*! Is there more data to be read? * - * @param[in] h Clixon handle - * @param[in] sd Restconf stream data (for http1 only stream 0) - * @retval 1 OK, message is fully read, proceed to processing - * @retval 0 OK, but message is partially read, need more data before processing - * @retval -1 Error + * Use Content-Length header as an indicator on the status of reading an input message: + * 0: No Content-Length or 0 + * Either message header not fully read OR header does not contain Content-Length + * 1: Content-Length found but body has fewer bytes, ie remaining bytes to read + * 2: Content-Length found and matches body length. No more bytes to read + * @param[in] h Clixon handle + * @param[in] sd Restconf stream data (for http1 only stream 0) + * @param[out] status 0-2, see description above + * @retval 0 OK, see status param + * @retval -1 Error */ int -http1_check_readmore(clicon_handle h, - restconf_stream_data *sd) +http1_check_content_length(clicon_handle h, + restconf_stream_data *sd, + int *status) { int retval = -1; char *val; int len; - if ((val = restconf_param_get(h, "HTTP_CONTENT_LENGTH")) != NULL && - (len = atoi(val)) != 0){ + if ((val = restconf_param_get(h, "HTTP_CONTENT_LENGTH")) == NULL || + (len = atoi(val)) == 0) + *status = 0; + else{ if (cbuf_len(sd->sd_indata) < len) - goto readmore; + *status = 1; + else + *status = 2; } - retval = 1; - done: - return retval; - readmore: retval = 0; - goto done; + return retval; } diff --git a/apps/restconf/restconf_http1.h b/apps/restconf/restconf_http1.h index cd53bf9a..bca39373 100644 --- a/apps/restconf/restconf_http1.h +++ b/apps/restconf/restconf_http1.h @@ -44,6 +44,6 @@ int clixon_http1_parse_string(clicon_handle h, restconf_conn *rc, char *str); int clixon_http1_parse_buf(clicon_handle h, restconf_conn *rc, char *buf, size_t n); int restconf_http1_path_root(clicon_handle h, restconf_conn *rc); int http1_check_expect(clicon_handle h, restconf_conn *rc, restconf_stream_data *sd); -int http1_check_readmore(clicon_handle h, restconf_stream_data *sd); +int http1_check_content_length(clicon_handle h, restconf_stream_data *sd, int *status); #endif /* _RESTCONF_HTTP1_H_ */ diff --git a/apps/restconf/restconf_native.c b/apps/restconf/restconf_native.c index e34a20fb..7c5e287d 100644 --- a/apps/restconf/restconf_native.c +++ b/apps/restconf/restconf_native.c @@ -499,6 +499,8 @@ restconf_connection(int s, #ifdef HAVE_HTTP1 clicon_handle h; restconf_stream_data *sd; + int status; + int http1_headers_read = 0; /* Dont re-parse headers, just append body */ #endif clicon_debug(1, "%s %d", __FUNCTION__, s); @@ -578,15 +580,34 @@ restconf_connection(int s, clicon_err(OE_RESTCONF, EINVAL, "restconf stream not found"); goto done; } - /* multi-buffer for multiple reads */ - totlen += n; - if ((totbuf = realloc(totbuf, totlen+1)) == NULL){ - clicon_err(OE_UNIX, errno, "realloc"); - goto done; + if (http1_headers_read){ + if (cbuf_append_buf(sd->sd_indata, buf, n) < 0){ + clicon_err(OE_UNIX, errno, "cbuf_append"); + goto done; + } } - memcpy(&totbuf[totlen-n], buf, n); - totbuf[totlen] = '\0'; - if (clixon_http1_parse_string(h, rc, totbuf) < 0){ + else { + /* multi-buffer for multiple reads */ + totlen += n; + if ((totbuf = realloc(totbuf, totlen+1)) == NULL){ + clicon_err(OE_UNIX, errno, "realloc"); + goto done; + } + memcpy(&totbuf[totlen-n], buf, n); + totbuf[totlen] = '\0'; + } + /* + * The following cases are handled after parsing + * - Parse error + * - More data to read? + * - clear and read more + * - No more data + * - send error + * - Parse OK + * - + */ + if (!http1_headers_read && + clixon_http1_parse_string(h, rc, totbuf) < 0){ /* Maybe only for non-ssl ? */ if ((ret = clixon_event_poll(rc->rc_s)) < 0) goto done; @@ -617,15 +638,31 @@ restconf_connection(int s, } } /* Check whole message is read. - * ret == 0: need more bytes + * Only way this could happen is that body is read + * 0: No Content-Length or 0 + * header does not contain Content-Length or is 0 + * (OR: message header not fully read SHOULDNT HAPPEN IF BODY READ) + * 1: Content-Length found but body has fewer bytes, ie remaining bytes to read + * 2: Content-Length found and matches body length. No more bytes to read */ - if ((ret = http1_check_readmore(h, sd)) < 0) + if ((ret = http1_check_content_length(h, sd, &status)) < 0) goto done; - if (ret == 0){ - if (native_clear_input(h, sd) < 0) - goto done; + switch (status){ + case 1: + /* Next read: keep header state and only append inbody */ + http1_headers_read++; readmore++; continue; + break; + + break; + case 0: + case 2: + default: + break; + } + if (status == 0){ + } if (restconf_http1_path_root(h, rc) < 0) goto done; diff --git a/test/test_perf_restconf.sh b/test/test_perf_restconf.sh index cb13fa36..1c9d73da 100755 --- a/test/test_perf_restconf.sh +++ b/test/test_perf_restconf.sh @@ -4,6 +4,9 @@ # Lists (and leaf-lists) # Add, get and delete entries +# Override default to use http/1.1 +HAVE_LIBNGHTTP2=false + # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -27,7 +30,7 @@ APPNAME=example cfg=$dir/scaling-conf.xml fyang=$dir/scaling.yang -fconfigonly=$dir/config.xml # only config for test +fdataxml=$dir/config.xml # dataxml ftest=$dir/test.xml fconfig=$dir/large.xml fconfig2=$dir/large2.xml # leaf-list @@ -108,28 +111,23 @@ wait_restconf # Check this later with committed data new "generate config with $perfnr list entries" -echo -n "" > $fconfigonly +echo -n "" > $fdataxml for (( i=0; i<$perfnr; i++ )); do - echo -n "$i$i" >> $fconfigonly + echo -n "$i$i" >> $fdataxml done -echo -n "" >> $fconfigonly # No CR +echo -n "" >> $fdataxml # No CR echo -n "$DEFAULTHELLO" > $fconfig -cat $fconfigonly >> $fconfig +cat $fdataxml >> $fconfig echo "]]>]]>" >> $fconfig # Now take large config file and write it via netconf to candidate new "test time exists" expectpart "$(time -p ls)" 0 -new "netconf write large config" -expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' - -# Here, there are $perfnr entries in candidate - -# Now commit it from candidate to running -new "netconf commit large config" -expecteof "time -p $clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' +new "restconf write large config" +expectpart "$(time -p curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+xml" $RCPROTO://localhost/restconf/data -d @$fdataxml )" 0 "HTTP/$HVER 201" +exit new "Check running-db contents" curl $CURLOPTS -X GET -H "Accept: application/yang-data+xml" $RCPROTO://localhost/restconf/data?content=config > $foutput @@ -166,13 +164,13 @@ echo "Content-Type: application/yang-data+xml " >> $ftest echo "Cache-Control: no-cache " >> $ftest echo " ">> $ftest echo -n "">> $ftest -cat $fconfigonly >> $ftest +cat $fdataxml >> $ftest echo " " >> $ftest ret=$(diff -i $ftest $foutput) if [ $? -ne 0 ]; then echo "diff -i $ftest $foutput" - err1 "Matching running-db with $fconfigonly" + err1 "Matching running-db with $fdataxml" fi # RESTCONF get diff --git a/test/test_restconf_continue.sh b/test/test_restconf_continue.sh index bc5d674a..c19e3b18 100755 --- a/test/test_restconf_continue.sh +++ b/test/test_restconf_continue.sh @@ -3,6 +3,7 @@ # Trigger Expect by curl -H. Some curls seem to trigger one on large PUTs but not all # Override default to use http/1.1 +# In http/2 there is no explicit continue HAVE_LIBNGHTTP2=false # Magic line must be first in script (see README.md)