From: Kirill A. Korinsky Subject: Re: relayd: move HTTP start-line logic to a dedicated function To: Rafael Sadowski Cc: tech@openbsd.org Date: Sat, 07 Mar 2026 13:43:51 +0100 On Sun, 01 Mar 2026 15:53:13 +0100, Rafael Sadowski wrote: > > Begin refactoring the oversized relay_read_http() aka 500+ lines by > moving HTTP start-line logic into a separate function. This is the first > of several planned extractions to reduce the complexity of the main read > loop. > > This is a structural refactoring with no functional change. > > Feedback? OK? > Make sense and reads OK kirill@ > diff --git a/usr.sbin/relayd/relay_http.c b/usr.sbin/relayd/relay_http.c > index e9b9c4449e5..4fafd4fdf6a 100644 > --- a/usr.sbin/relayd/relay_http.c > +++ b/usr.sbin/relayd/relay_http.c > @@ -78,6 +78,8 @@ int relay_match_actions(struct ctl_relay_event *, > struct relay_table **); > void relay_httpdesc_free(struct http_descriptor *); > char * server_root_strip(char *, int); > +int relay_http_parse_startline(struct ctl_relay_event *, char *, > + enum httpmethod *); > > static struct relayd *env = NULL; > > @@ -193,8 +195,6 @@ relay_read_http(struct bufferevent *bev, void *arg) > struct kv *upgrade = NULL, *upgrade_ws = NULL; > struct kv *connection_close = NULL; > int ws_response = 0; > - struct http_method_node *hmn; > - struct http_session *hs; > enum httpmethod request_method = HTTP_METHOD_NONE; > > getmonotime(&con->se_tv_last); > @@ -248,137 +248,13 @@ relay_read_http(struct bufferevent *bev, void *arg) > relay_abort_http(con, 400, "malformed", 0); > goto abort; > } > - > - hs = con->se_priv; > - DPRINTF("%s: session %d http_session %p", __func__, > - con->se_id, hs); > - > - /* > - * The first line is the GET/POST/PUT/... request, > - * subsequent lines are HTTP headers. > - */ > + /* Process the HTTP start-line (request or response status) */ > if (++cre->line == 1) { > - key = line; > - if ((value = strchr(key, ' ')) == NULL) { > - relay_abort_http(con, 400, "malformed", 0); > - goto abort; > - } > - *value++ = '\0'; > - > - if (cre->dir == RELAY_DIR_RESPONSE) { > - desc->http_method = HTTP_METHOD_RESPONSE; > - hmn = SIMPLEQ_FIRST(&hs->hs_methods); > - > - /* > - * There is nothing preventing the relay from > - * sending an unbalanced response. Be prepared. > - */ > - if (hmn == NULL) { > - request_method = HTTP_METHOD_NONE; > - DPRINTF("%s: session %d unbalanced " > - "response", __func__, con->se_id); > - } else { > - SIMPLEQ_REMOVE_HEAD(&hs->hs_methods, > - hmn_entry); > - request_method = hmn->hmn_method; > - DPRINTF("%s: session %d dequeuing %s", > - __func__, con->se_id, > - relay_httpmethod_byid(request_method)); > - free(hmn); > - } > - > - /* > - * Decode response path and query > - */ > - desc->http_version = strdup(key); > - if (desc->http_version == NULL) { > - free(line); > - goto fail; > - } > - desc->http_rescode = strdup(value); > - if (desc->http_rescode == NULL) { > - free(line); > - goto fail; > - } > - desc->http_resmesg = strchr(desc->http_rescode, > - ' '); > - if (desc->http_resmesg == NULL) { > - free(line); > - goto fail; > - } > - *desc->http_resmesg++ = '\0'; > - desc->http_resmesg = strdup(desc->http_resmesg); > - if (desc->http_resmesg == NULL) { > - free(line); > - goto fail; > - } > - desc->http_status = strtonum(desc->http_rescode, > - 100, 599, &errstr); > - if (errstr) { > - DPRINTF( > - "%s: http_status %s: errno %d, %s", > - __func__, desc->http_rescode, errno, > - errstr); > - free(line); > - goto fail; > - } > - DPRINTF("http_version %s http_rescode %s " > - "http_resmesg %s", desc->http_version, > - desc->http_rescode, desc->http_resmesg); > - } else if (cre->dir == RELAY_DIR_REQUEST) { > - desc->http_method = > - relay_httpmethod_byname(key); > - if (desc->http_method == HTTP_METHOD_NONE) { > - free(line); > - goto fail; > - } > - if ((hmn = calloc(1, sizeof *hmn)) == NULL) { > - free(line); > - goto fail; > - } > - hmn->hmn_method = desc->http_method; > - DPRINTF("%s: session %d enqueuing %s", > - __func__, con->se_id, > - relay_httpmethod_byid(hmn->hmn_method)); > - SIMPLEQ_INSERT_TAIL(&hs->hs_methods, hmn, > - hmn_entry); > - /* > - * Decode request path and query > - */ > - desc->http_path = strdup(value); > - if (desc->http_path == NULL) { > - free(line); > - goto fail; > - } > - desc->http_version = strchr(desc->http_path, > - ' '); > - if (desc->http_version == NULL) { > - free(line); > - goto fail; > - } > - *desc->http_version++ = '\0'; > - desc->http_query = strchr(desc->http_path, '?'); > - if (desc->http_query != NULL) > - *desc->http_query++ = '\0'; > - > - /* > - * Have to allocate the strings because they > - * could be changed independently by the > - * filters later. > - */ > - if ((desc->http_version = > - strdup(desc->http_version)) == NULL) { > - free(line); > - goto fail; > - } > - if (desc->http_query != NULL && > - (desc->http_query = > - strdup(desc->http_query)) == NULL) { > - free(line); > - goto fail; > - } > + if (relay_http_parse_startline(cre, line, > + &request_method) == -1) { > + free(line); > + return; > } > - > free(line); > continue; > } > @@ -2127,3 +2003,124 @@ server_root_strip(char *path, int n) > return (path); > } > > +/* > + * Handle the first line of an HTTP message (Request- or Status-Line). > + */ > +int > +relay_http_parse_startline(struct ctl_relay_event *cre, char *line, > + enum httpmethod *request_method) > +{ > + struct rsession *con = cre->con; > + struct http_descriptor *desc = cre->desc; > + struct http_session *hs = con->se_priv; > + struct http_method_node *hmn; > + char *key, *value; > + const char *errstr; > + > + DPRINTF("%s: session %d http_session %p", __func__, con->se_id, hs); > + > + key = line; > + if ((value = strchr(key, ' ')) == NULL) { > + relay_abort_http(con, 400, "malformed", 0); > + return (-1); > + } > + *value++ = '\0'; > + > + if (cre->dir == RELAY_DIR_RESPONSE) { > + desc->http_method = HTTP_METHOD_RESPONSE; > + hmn = SIMPLEQ_FIRST(&hs->hs_methods); > + > + /* > + * There is nothing preventing the relay from > + * sending an unbalanced response. Be prepared. > + */ > + if (hmn == NULL) { > + *request_method = HTTP_METHOD_NONE; > + DPRINTF("%s: session %d unbalanced response", > + __func__, con->se_id); > + } else { > + SIMPLEQ_REMOVE_HEAD(&hs->hs_methods, hmn_entry); > + *request_method = hmn->hmn_method; > + DPRINTF("%s: session %d dequeuing %s", > + __func__, con->se_id, > + relay_httpmethod_byid(*request_method)); > + free(hmn); > + } > + > + /* > + * Decode response path and query > + */ > + desc->http_version = strdup(key); > + if (desc->http_version == NULL) > + goto fail; > + > + desc->http_rescode = strdup(value); > + if (desc->http_rescode == NULL) > + goto fail; > + > + desc->http_resmesg = strchr(desc->http_rescode, ' '); > + if (desc->http_resmesg == NULL) > + goto fail; > + > + *desc->http_resmesg++ = '\0'; > + desc->http_resmesg = strdup(desc->http_resmesg); > + if (desc->http_resmesg == NULL) > + goto fail; > + > + desc->http_status = strtonum(desc->http_rescode, 100, 599, > + &errstr); > + if (errstr) { > + DPRINTF( > + "%s: http_status %s: errno %d, %s", > + __func__, desc->http_rescode, errno, > + errstr); > + goto fail; > + } > + DPRINTF("http_version %s http_rescode %s http_resmesg %s", > + desc->http_version, desc->http_rescode, > + desc->http_resmesg); > + } else if (cre->dir == RELAY_DIR_REQUEST) { > + desc->http_method = relay_httpmethod_byname(key); > + > + if (desc->http_method == HTTP_METHOD_NONE) > + goto fail; > + > + if ((hmn = calloc(1, sizeof *hmn)) == NULL) > + goto fail; > + hmn->hmn_method = desc->http_method; > + DPRINTF("%s: session %d enqueuing %s", __func__, con->se_id, > + relay_httpmethod_byid(hmn->hmn_method)); > + SIMPLEQ_INSERT_TAIL(&hs->hs_methods, hmn, hmn_entry); > + /* > + * Decode request path and query > + */ > + desc->http_path = strdup(value); > + if (desc->http_path == NULL) > + goto fail; > + > + desc->http_version = strchr(desc->http_path, ' '); > + if (desc->http_version == NULL) > + goto fail; > + > + *desc->http_version++ = '\0'; > + desc->http_query = strchr(desc->http_path, '?'); > + if (desc->http_query != NULL) > + *desc->http_query++ = '\0'; > + > + /* > + * Have to allocate the strings because they > + * could be changed independently by the > + * filters later. > + */ > + if ((desc->http_version = strdup(desc->http_version)) == NULL) > + goto fail; > + > + if (desc->http_query != NULL && (desc->http_query = > + strdup(desc->http_query)) == NULL) > + goto fail; > + } > + return (0); > +fail: > + relay_abort_http(con, 500, strerror(errno), 0); > + return (-1); > +} > -- wbr, Kirill