Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom HTTP redirects (Unification) #1628

Merged
merged 2 commits into from
May 31, 2022
Merged

Custom HTTP redirects (Unification) #1628

merged 2 commits into from
May 31, 2022

Conversation

avbelov23
Copy link
Contributor

#856

Remark:
The body of the js challenge redirect can consist of chunks (tfw_http_sticky_build_redirect()), but tfw_h2_frame_local_resp() is not ready to work with such a body, therefore, to avoid problems, it was temporarily decided to expand such a body into a plain tfw string

@avbelov23 avbelov23 requested a review from krizhanovsky May 16, 2022 21:47
@avbelov23 avbelov23 force-pushed the avb-856 branch 5 times, most recently from 70b8716 to 30de466 Compare May 18, 2022 14:51
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the effort to aggregate all the response functions - the code now looks much better! There are still places to improve and/or discuss. Also please see new comments in #1618

fw/http_tbl.c Outdated Show resolved Hide resolved
fw/http.c Outdated Show resolved Hide resolved
fw/http_tbl.c Outdated Show resolved Hide resolved
fw/http_tbl.c Show resolved Hide resolved
fw/http.c Outdated Show resolved Hide resolved
fw/http.c Outdated
}
body_len = tfw_str_to_cstr(body, body_val, BODY_BUF_LEN);
if (!body_len)
return TFW_BLOCK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, tfw_h1_prep_resp() -> tfw_msg_write() and tfw_h2_prep_resp() -> tfw_h2_frame_local_resp() -> tfw_h2_append_predefined_body() do allocate skb memory and copy the msg chunks into it, so we're good with many chunks at this layer, so we can just append body chunks to msg and don't need to copy them

I see your comment

Remark:
The body of the js challenge redirect can consist of chunks (tfw_http_sticky_build_redirect()), but tfw_h2_frame_local_resp() is not ready to work with such a body, therefore, to avoid problems, it was temporarily decided to expand such a body into a plain tfw string

, but tfw_h2_frame_local_resp() accepts body as TfwStr, which confuses me. Let's discuss this on the call.

Copy link
Contributor Author

@avbelov23 avbelov23 May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tfw_h2_append_predefined_body() for some reason expects a plain string

The function is designed for @body preallocated during
configuration processing thus no chunked body allowed, only plain TfwStr is accepted there

fw/http.c Outdated Show resolved Hide resolved
fw/http_sess.c Show resolved Hide resolved
fw/http.c Outdated Show resolved Hide resolved
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made several minor code cleanups and add comment by our discussions.

@krizhanovsky krizhanovsky merged commit a5f5fb4 into master May 31, 2022
@krizhanovsky krizhanovsky deleted the avb-856 branch May 31, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants