From f2810e849f558b8cf85115a3ac7e0d3af46f79a1 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sun, 4 Nov 2018 14:46:15 +0100 Subject: [PATCH] Fork fcgi handler for streams --- CHANGELOG.md | 1 + apps/restconf/restconf_lib.c | 22 ++++- apps/restconf/restconf_lib.h | 1 + apps/restconf/restconf_main.c | 62 +++++++------ apps/restconf/restconf_methods.c | 2 +- apps/restconf/restconf_stream.c | 144 ++++++++++++++++++++++++++----- apps/restconf/restconf_stream.h | 4 +- example/example_restconf.c | 2 +- test/lib.sh | 2 +- test/test_stream.sh | 21 ++++- 10 files changed, 208 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e40c94a..ace3179b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ * Restconf stream notification support according to RFC8040 * See (apps/restconf/README.md) for more details. * start-time and stop-time query parameters + * Fork fcgi handler for streams * Set access/subscribe base URL with: CLICON_STREAM_URL (default "https://localhost") and CLICON_STREAM_PATH (default "streams") * Example: new stream "foo" will get access URL: https://localhost/streams/foo * Alternative variant using pub/sub support enabled by ./configure --enable-publish diff --git a/apps/restconf/restconf_lib.c b/apps/restconf/restconf_lib.c index 0ac75dc3..75c06699 100644 --- a/apps/restconf/restconf_lib.c +++ b/apps/restconf/restconf_lib.c @@ -54,7 +54,7 @@ /* clicon */ #include -#include /* Need to be after clixon_xml-h due to attribute format */ +#include /* Need to be after clixon_xml-h due to attribute format */ #include "restconf_lib.h" @@ -485,3 +485,23 @@ api_return_err(clicon_handle h, cbuf_free(cb); return retval; } + +int +restconf_terminate(clicon_handle h) +{ + yang_spec *yspec; + cxobj *x; + + clixon_plugin_exit(h); + rpc_callback_delete_all(); + clicon_rpc_close_session(h); + if ((yspec = clicon_dbspec_yang(h)) != NULL) + yspec_free(yspec); + if ((yspec = clicon_config_yang(h)) != NULL) + yspec_free(yspec); + if ((x = clicon_conf_xml(h)) != NULL) + xml_free(x); + clicon_handle_exit(h); + clicon_log_exit(); + return 0; +} diff --git a/apps/restconf/restconf_lib.h b/apps/restconf/restconf_lib.h index 0c7f6830..e05eb054 100644 --- a/apps/restconf/restconf_lib.h +++ b/apps/restconf/restconf_lib.h @@ -62,5 +62,6 @@ cbuf *readdata(FCGX_Request *r); int get_user_cookie(char *cookiestr, char *attribute, char **val); int api_return_err(clicon_handle h, FCGX_Request *r, cxobj *xerr, int pretty, int use_xml); +int restconf_terminate(clicon_handle h); #endif /* _RESTCONF_LIB_H_ */ diff --git a/apps/restconf/restconf_main.c b/apps/restconf/restconf_main.c index b931c63e..9ee20fd4 100644 --- a/apps/restconf/restconf_main.c +++ b/apps/restconf/restconf_main.c @@ -73,7 +73,7 @@ /* clicon */ #include -#include /* Need to be after clixon_xml.h due to attribute format */ +#include /* Need to be after clixon_xml.h due to attribute format */ /* restconf */ #include "restconf_lib.h" @@ -443,27 +443,8 @@ api_restconf(clicon_handle h, return retval; } -static int -restconf_terminate(clicon_handle h) -{ - yang_spec *yspec; - cxobj *x; - clixon_plugin_exit(h); - rpc_callback_delete_all(); - clicon_rpc_close_session(h); - if ((yspec = clicon_dbspec_yang(h)) != NULL) - yspec_free(yspec); - if ((yspec = clicon_config_yang(h)) != NULL) - yspec_free(yspec); - if ((x = clicon_conf_xml(h)) != NULL) - xml_free(x); - clicon_handle_exit(h); - clicon_log_exit(); - return 0; -} - -/* Need global variable to for signal handler */ +/* Need global variable to for signal handler XXX */ static clicon_handle _CLICON_HANDLE = NULL; /*! Signall terminates process @@ -478,12 +459,24 @@ restconf_sig_term(int arg) __PROGRAM__, __FUNCTION__, getpid(), arg); else exit(-1); - if (_CLICON_HANDLE) + if (_CLICON_HANDLE){ + stream_child_freeall(_CLICON_HANDLE); restconf_terminate(_CLICON_HANDLE); + } clicon_exit_set(); /* checked in event_loop() */ exit(-1); } +static void +restconf_sig_child(int arg) +{ + int status; + int pid; + + if ((pid = waitpid(-1, &status, 0)) != -1 && WIFEXITED(status)) + stream_child_free(_CLICON_HANDLE, pid); +} + /*! Usage help routine * @param[in] argv0 command line * @param[in] h Clicon handle @@ -532,6 +525,7 @@ main(int argc, yang_spec *yspecfg = NULL; /* For config XXX clixon bug */ char *yang_filename = NULL; char *stream_path; + int finish; /* In the startup, logs to stderr & debug flag set later */ clicon_log_init(__PROGRAM__, LOG_INFO, logdst); @@ -579,6 +573,11 @@ main(int argc, clicon_err(OE_DEMON, errno, "Setting signal"); goto done; } + if (set_signal(SIGCHLD, restconf_sig_child, NULL) < 0){ + clicon_err(OE_DEMON, errno, "Setting signal"); + goto done; + } + /* Create configure yang-spec */ if ((yspecfg = yspec_new()) == NULL) goto done; @@ -663,11 +662,12 @@ main(int argc, clixon_plugin_start(h, argc+1, argv-1); *(argv-1) = tmp; + /**/ if ((sockpath = clicon_option_str(h, "CLICON_RESTCONF_PATH")) == NULL){ clicon_err(OE_CFG, errno, "No CLICON_RESTCONF_PATH in clixon configure file"); goto done; } - if (FCGX_Init() != 0){ + if (FCGX_Init() != 0){ /* How to cleanup memory after this? */ clicon_err(OE_CFG, errno, "FCGX_Init"); goto done; } @@ -676,12 +676,13 @@ main(int argc, clicon_err(OE_CFG, errno, "FCGX_OpenSocket"); goto done; } - if (FCGX_InitRequest(r, sock, 0) != 0){ clicon_err(OE_CFG, errno, "FCGX_InitRequest"); goto done; } while (1) { + finish = 1; /* If zero, dont finish request, initiate new */ + if (FCGX_Accept_r(r) < 0) { clicon_err(OE_CFG, errno, "FCGX_Accept_r"); goto done; @@ -692,7 +693,7 @@ main(int argc, if (strncmp(path, "/" RESTCONF_API, strlen("/" RESTCONF_API)) == 0) api_restconf(h, r); /* This is the function */ else if (strncmp(path+1, stream_path, strlen(stream_path)) == 0) { - api_stream(h, r, stream_path); + api_stream(h, r, stream_path, &finish); } else if (strncmp(path, RESTCONF_WELL_KNOWN, strlen(RESTCONF_WELL_KNOWN)) == 0) { api_well_known(h, r); /* */ @@ -704,10 +705,19 @@ main(int argc, } else clicon_debug(1, "NULL URI"); - FCGX_Finish_r(r); + if (finish) + FCGX_Finish_r(r); + else{ /* A handler is forked so we initiate a new request after instead + of finnishing the old */ + if (FCGX_InitRequest(r, sock, 0) != 0){ + clicon_err(OE_CFG, errno, "FCGX_InitRequest"); + goto done; + } + } } retval = 0; done: + stream_child_freeall(h); restconf_terminate(h); return retval; } diff --git a/apps/restconf/restconf_methods.c b/apps/restconf/restconf_methods.c index 6a1712f6..133f0ed9 100644 --- a/apps/restconf/restconf_methods.c +++ b/apps/restconf/restconf_methods.c @@ -118,7 +118,7 @@ Mapping netconf error-tag -> status code /* clicon */ #include -#include /* Need to be after clixon_xml-h due to attribute format */ +#include /* Need to be after clixon_xml-h due to attribute format */ #include "restconf_lib.h" #include "restconf_methods.h" diff --git a/apps/restconf/restconf_stream.c b/apps/restconf/restconf_stream.c index fa9b1ec3..ba47d75e 100644 --- a/apps/restconf/restconf_stream.c +++ b/apps/restconf/restconf_stream.c @@ -82,11 +82,71 @@ /* clicon */ #include -#include /* Need to be after clixon_xml.h due to attribute format */ +#include /* Need to be after clixon_xml.h due to attribute format */ #include "restconf_lib.h" #include "restconf_stream.h" +/* + * Constants + */ +/* Enable for forking stream subscription loop. + * Disable to get single threading but blocking on streams + */ +#define STREAM_FORK 1 + +/* Keep track of children - whjen they exit - their FCGX handle needs to be + * freed with FCGX_Free(&rbk, 0); + */ +struct stream_child{ + qelem_t sc_q; /* queue header */ + int sc_pid; /* Child process id */ + FCGX_Request sc_r; /* FCGI stream data */ +}; +/* Linked list of children + * @note could hang STREAM_CHILD list on clicon handle instead. + */ +static struct stream_child *STREAM_CHILD = NULL; + +/*! Find restconf child using PID and cleanup FCGI Request data + * @param[in] h Clicon handle + * @param[in] pid Process id of child + * @note could hang STREAM_CHILD list on clicon handle instead. + */ +int +stream_child_free(clicon_handle h, + int pid) +{ + struct stream_child *sc; + + if ((sc = STREAM_CHILD) != NULL){ + do { + if (pid == sc->sc_pid){ + DELQ(sc, STREAM_CHILD, struct stream_child *); + FCGX_Free(&sc->sc_r, 0); + free(sc); + goto done; + } + sc = NEXTQ(struct stream_child *, sc); + } while (sc && sc != STREAM_CHILD); + } + done: + return 0; +} + +int +stream_child_freeall(clicon_handle h) +{ + struct stream_child *sc; + + while ((sc = STREAM_CHILD) != NULL){ + DELQ(sc, STREAM_CHILD, struct stream_child *); + FCGX_Free(&sc->sc_r, 1); + free(sc); + } + return 0; +} + /*! Callback when stream notifications arrive from backend */ static int @@ -278,7 +338,8 @@ stream_timeout(int s, int api_stream(clicon_handle h, FCGX_Request *r, - char *streampath) + char *streampath, + int *finish) { int retval = -1; char *path; @@ -298,6 +359,10 @@ api_stream(clicon_handle h, cxobj *xret = NULL; cxobj *xerr; int s=-1; +#ifdef STREAM_FORK + int pid; + struct stream_child *sc; +#endif clicon_debug(1, "%s", __FUNCTION__); path = FCGX_GetParam("DOCUMENT_URI", r->envp); @@ -362,24 +427,64 @@ api_stream(clicon_handle h, if (restconf_stream(h, r, method, qvec, pretty, use_xml, &s) < 0) goto done; if (s != -1){ - /* Listen to backend socket */ - if (event_reg_fd(s, - restconf_stream_cb, - (void*)r, - "stream socket") < 0) +#ifdef STREAM_FORK + if ((pid = fork()) == 0){ /* child */ + if (pvec) + free(pvec); + if (dvec) + cvec_free(dvec); + if (qvec) + cvec_free(qvec); + if (pcvec) + cvec_free(pcvec); + if (cb) + cbuf_free(cb); + if (cbret) + cbuf_free(cbret); + if (xret) + xml_free(xret); +#endif /* STREAM_FORK */ + /* Listen to backend socket */ + if (event_reg_fd(s, + restconf_stream_cb, + (void*)r, + "stream socket") < 0) + goto done; + if (event_reg_fd(r->listen_sock, + stream_checkuplink, + (void*)r, + "stream socket") < 0) + goto done; + /* Poll upstream errors */ + stream_timeout(0, (void*)r); + /* Start loop */ + event_loop(); + close(s); + event_unreg_fd(s, restconf_stream_cb); + event_unreg_fd(r->listen_sock, restconf_stream_cb); + event_unreg_timeout(stream_timeout, (void*)r); + clicon_exit_reset(); +#ifdef STREAM_FORK + FCGX_Finish_r(r); + FCGX_Free(r, 0); + fprintf(stderr, "child exit and free\n"); + restconf_terminate(h); + exit(0); + } + /* parent */ + /* Create stream_child struct and store pid and FCGI data, when child + * killed, call FCGX_Free + */ + if ((sc = malloc(sizeof(struct stream_child))) == NULL){ + clicon_err(OE_XML, errno, "malloc"); goto done; - if (event_reg_fd(r->listen_sock, - stream_checkuplink, - (void*)r, - "stream socket") < 0) - goto done; - /* Poll upstream errors */ - stream_timeout(0, (void*)r); - /* Start loop */ - event_loop(); - close(s); - event_unreg_fd(s, restconf_stream_cb); - clicon_exit_reset(); + } + memset(sc, 0, sizeof(struct stream_child)); + sc->sc_pid = pid; + sc->sc_r = *r; + ADDQ(sc, STREAM_CHILD); + *finish = 0; /* If spawn child, we should not finish this stream */ +#endif /* STREAM_FORK */ } ok: retval = 0; @@ -399,6 +504,5 @@ api_stream(clicon_handle h, cbuf_free(cbret); if (xret) xml_free(xret); - return retval; } diff --git a/apps/restconf/restconf_stream.h b/apps/restconf/restconf_stream.h index bae59da6..480832f1 100644 --- a/apps/restconf/restconf_stream.h +++ b/apps/restconf/restconf_stream.h @@ -39,6 +39,8 @@ /* * Prototypes */ -int api_stream(clicon_handle h, FCGX_Request *r, char *streampath); +int stream_child_free(clicon_handle h, int pid); +int stream_child_freeall(clicon_handle h); +int api_stream(clicon_handle h, FCGX_Request *r, char *streampath, int *finish); #endif /* _RESTCONF_STREAM_H_ */ diff --git a/example/example_restconf.c b/example/example_restconf.c index 2b22ce16..6cc6323d 100644 --- a/example/example_restconf.c +++ b/example/example_restconf.c @@ -39,7 +39,7 @@ #include #include #include -#include +#include #include /* cligen */ diff --git a/test/lib.sh b/test/lib.sh index 00cb63c9..a518baf3 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -18,7 +18,7 @@ clixon_netconf=clixon_netconf # How to run restconf stand-alone and using valgrind #sudo su -c "/www-data/clixon_restconf -f $cfg -D 1" -s /bin/sh www-data -#sudo su -c "valgrind --leak-check=full --show-leak-kinds=all /www-data/clixon_restconf -f $cfg -D 1" -s /bin/sh www-data +#sudo su -c "valgrind --trace-children=no --child-silent-after-fork=yes --leak-check=full --show-leak-kinds=all /www-data/clixon_restconf -f $cfg -D 1" -s /bin/sh www-data #clixon_backend="valgrind --leak-check=full --show-leak-kinds=all clixon_backend" clixon_backend=clixon_backend diff --git a/test/test_stream.sh b/test/test_stream.sh index b00d53fd..3ee3cc6b 100755 --- a/test/test_stream.sh +++ b/test/test_stream.sh @@ -111,7 +111,8 @@ new "kill old restconf daemon" sudo pkill -u www-data clixon_restconf new "start restconf daemon" -sudo start-stop-daemon -S -q -o -b -x /www-data/clixon_restconf -d /www-data -c www-data -- -f $cfg -y $fyang # -D 1 + +sudo su -c "/www-data/clixon_restconf -f $cfg -y $fyang" -s /bin/sh www-data & sleep 2 @@ -167,7 +168,6 @@ sleep 2 new "restconf monitor event nonexist stream" expectwait 'curl -s -X GET -H "Accept: text/event-stream" -H "Cache-Control: no-cache" -H "Connection: keep-alive" http://localhost/streams/NOTEXIST' 0 'invalid-valueapplicationerrorNo such stream' 2 - # 2a) start subscription 8s - expect 1-2 notifications new "2a) start subscriptions 8s - expect 1-2 notifications" ret=$($UTIL -u http://localhost/streams/EXAMPLE -t 8) @@ -236,6 +236,23 @@ if [ $nr -lt 10 -o $nr -gt 14 ]; then err 10 "$nr" fi +# Try parallell +# start background job +curl -s -X GET -H "Accept: text/event-stream" -H "Cache-Control: no-cache" -H "Connection: keep-alive" "http://localhost/streams/EXAMPLE" > /dev/null & + +new "Start subscription in parallell" +ret=$($UTIL -u http://localhost/streams/EXAMPLE -t 8) +expect="data: ${DATE}T[0-9:.]*faultEthernet0major" + +match=$(echo "$ret" | grep -Eo "$expect") +if [ -z "$match" ]; then + err "$expect" "$ret" +fi +nr=$(echo "$ret" | grep -c "data:") +if [ $nr -lt 1 -o $nr -gt 2 ]; then + err 2 "$nr" +fi + #----------------- sudo pkill -u www-data clixon_restconf