In D53005#1212138, @asomers wrote:

I too am confused by the commit message. Won't the column always be shown in json and xml output, if "-s" is used?

In D52989#1210787, @gallatin wrote:

Also, this needs to be MFC'ed to 15 for the release..

In D52984#1210640, @asomers wrote:

Could you please give an example of the before and after output?

In D52986#1210638, @asomers wrote:

Could you give an example of what the output looks like before and after?

In D52934#1209636, @netchild wrote:

I can confirm that this fixes the crash I've seen. Instead of crashing after a few minutes, it now is still humming happily with 16 minutes of uptime.

Address one of Jonathan's comments.

In D52872#1207881, @jtl wrote:

Is this worthy of a release note?

In D52816#1206994, @glebius wrote:

I agree with Nick that previous comments were better.

I can expand the variable names. But I would like to be consistent in stating what is tested: in one test SEG.SEQ, and in the other SEG.ACK.

You may add assertions that match the checks in the syncookie_expand() case.

I will take this into account when the code is doing the actions it should.

Also, I'm not sure "not a functional change" applies in the commit message. TCP wise there is no change, but code wise there is.

That is not there anymore...

P.S. Interesting that the email generator of phabricator produced a much easier diff to read than the web version.

I totally agree....

In D52816#1206939, @nickbanks_netflix.com wrote:

This change looks good to me, though I personally prefer to the more verbose comments before you shortened them. But I wouldn't block on that.

OK. Moving the checks to the proper place is the first step. The second step will be to do the right action, which is sending a challenge ACK instead of killing the entry. When writing the comments for the second step, I will expand the variable names again.

One comment about the preexisting pattern here: requiring every failure path to individually unlock is error prone IMHO, as well as having a mix of return and goto. There's already a locked variable too. I don't think a single exit point with the cleanup would be too hard to add... just a thought.

Once we have the right functionality, we can cleanup the function. I just did not wanted to mix these things up.