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

Bugfix/nicps 492 #95

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Bugfix/nicps 492 #95

wants to merge 2 commits into from

Conversation

zimsuchitgupta
Copy link

No description provided.

@Prashantsurana Prashantsurana requested review from thefoolwhocodes, gordyt and fatkudu and removed request for thefoolwhocodes December 11, 2018 14:04
@gordyt
Copy link
Contributor

gordyt commented Dec 11, 2018

@zimsuchitgupta a couple of quick questions...

  1. What testing has been done with these changes?
  2. Have you run with valgrind to check for memory leaks?

@thefoolwhocodes
Copy link
Contributor

Hi @zimsuchitgupta a PR description can be handy.
A suitable format i can suggest is

Problem:

  • ABC

Approach and Fix:

  • XYZ

Testing done:

  • Sanity
  • Valgrind
  • XYZ

@jeastman
Copy link

Some context for this pull request:

This patch was developed to specifically handle the NetScaler TCP header for client IP, which is injected into the TCP stream. It has been validated using the NetScaler virtual appliance. Without this support there is no way for Nginx to obtain the originating client IP address as requests are forwarded from the NetScaler.

gordyt
gordyt approved these changes Dec 13, 2018
View reviewed changes
Copy link
Contributor

@gordyt gordyt left a comment

Choose a reason for hiding this comment

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

@jeastman - The changes look fine. I had asked if any testing with a tool like valgrind had been done to verify no memory leaks -- always a good thing to do for any nginx updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gordyt gordyt gordyt approved these changes

@thefoolwhocodes thefoolwhocodes Awaiting requested review from thefoolwhocodes

@fatkudu fatkudu Awaiting requested review from fatkudu

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zimsuchitgupta @gordyt @thefoolwhocodes @jeastman