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.

Sign up for GitHub

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

Jump to bottom

HTTP/2, header conversion tightening #12097

Closed
wants to merge 3 commits into from
Closed

HTTP/2, header conversion tightening #12097

wants to merge 3 commits into from

Conversation

icing
Copy link
Contributor

@icing icing commented Oct 12, 2023

  • fold the code to convert dynhds to the nghttp2 structs into a dynhds internal method
  • saves code duplication
  • pacifies compiler analyzers

@icing icing requested review from jay and vszakats October 12, 2023 08:17
@icing icing added the tidy-up label Oct 12, 2023
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 12, 2023
@vszakats
test curl PR 12097
978eb49 
Ref: curl/curl#12097
@vszakats
Copy link
Member

vszakats commented Oct 12, 2023 •
edited

Warnings reduced to a single block after the patch.

Before:
https://github.com/curl/curl-for-win/actions/runs/6499470079/job/17652877625#step:3:5316

After:
https://github.com/curl/curl-for-win/actions/runs/6499706712/job/17655051866#step:3:5324

/Users/runner/work/curl-for-win/curl-for-win/curl/lib/vquic/curl_ngtcp2.c:1709:38: warning: potential null pointer dereference [-Wnull-dereference]
 1709 |     nva[i].value = (unsigned char *)e->value;
      |                                     ~^~~~~~~
/Users/runner/work/curl-for-win/curl-for-win/curl/lib/vquic/curl_ngtcp2.c:1708:23: warning: potential null pointer dereference [-Wnull-dereference]
 1708 |     nva[i].namelen = e->namelen;
      |                      ~^~~~~~~~~
/Users/runner/work/curl-for-win/curl-for-win/curl/lib/vquic/curl_ngtcp2.c:1707:37: warning: potential null pointer dereference [-Wnull-dereference]
 1707 |     nva[i].name = (unsigned char *)e->name;
      |                                    ~^~~~~~
/Users/runner/work/curl-for-win/curl-for-win/curl/lib/vquic/curl_ngtcp2.c:1710:24: warning: potential null pointer dereference [-Wnull-dereference]
 1710 |     nva[i].valuelen = e->valuelen;
      |                       ~^~~~~~~~~~

@jay
Copy link
Member

jay commented Oct 13, 2023

That's also true of msh3 and quiche. I think the pattern could be eliminated from all of vquic. Otherwise it looks fine.

bagder
bagder reviewed Oct 13, 2023
View reviewed changes
lib/dynhds.c Outdated Show resolved Hide resolved
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 16, 2023
@vszakats
test curl PR 12097
de80d44 
Ref: curl/curl#12097
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 17, 2023
@vszakats
test curl PR 12097
2b4bd96 
Ref: curl/curl#12097
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 18, 2023
@vszakats
test curl PR 12097
2b06037 
Ref: curl/curl#12097
@icing
HTTP/2, header conversion tightening
e03d15b 
- fold the code to convert dynhds to the nghttp2 structs
  into a dynhds internal method
- saves code duplication
- pacifies compiler analyzers
@icing
remove copynpaste includes from dynhds.c
bfce7ce
@icing
assign with local var as requested by bagder
e80b726
@bagder bagder closed this in 117c9bd Oct 21, 2023
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 22, 2023
@vszakats
test curl PR 12097
5ca072c 
Ref: curl/curl#12097
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder left review comments

@jay jay Awaiting requested review from jay

@vszakats vszakats Awaiting requested review from vszakats

Assignees
No one assigned
Labels
hacktoberfest-accepted tidy-up
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@icing @vszakats @jay @bagder