Hi William, Thank you for making the changes to the first 2 patches. I apologize for not paying too much attention to the tab/spaces on the first patch. I did not realise the first patch would get merged (I thought it was just to help review patch-3). I worked on the indentation issue on patch-3. I hope that will have less issue with indentation. Changes to the test file looks fine to me. Thank you for making the changes and I am perfectly fine with merging this two files.
Appreciate your help. Regards, Mariam. From: William Lallemand <wlallem...@haproxy.com> Date: Thursday, April 17, 2025 at 5:58 AM To: Mariam John <john.mariam....@gmail.com> Cc: haproxy@formilux.org <haproxy@formilux.org> Subject: [EXTERNAL] Re: [PATCH 0/3] Add 4 new sample fetches to get ciphers, supported groups, key shares and sigalgs from ClientHello message Hello Mariam, On Wed, Apr 16, 2025 at 08:36:06AM -0500, Mariam John wrote: > Subject: [PATCH 0/3] Add 4 new sample fetches to get ciphers, supported > groups, key shares and sigalgs from ClientHello message > Hello William, > > Thank you for your patience and valuable feedback and reviews. Appreciate > it. As you requested, I have > broken the changes into 3 seperate patches to help with the review process. > The first patch contains the > 4 new fetches + the doc changes as I had originally submitted (with all the > review comments addressed). > The second patch contains just the new regression test added for the 4 new > fetches (with the review > comments addressed). There were still a lot of spaces and tabs issues that I fixed and I changed the subject to match our contributing guide ( https://github.com/haproxy/haproxy/blob/master/CONTRIBUTING#L632 ) in the first patch. You should really look at the documentation of your editor to fix that, we are using tabs for indentation and spaces for alignment. Also we have a documentation which explain it there: https://www.haproxy.org/coding-style.html The reg-test had a small issue because we are more strict and now start with haproxy -dW (no warning), so I removed the useless tcplog lines which was doing the warning. I put the 2 patches in attachment, with the fixed alignment and indentation, I can merge them if you are fine with the changes. > The third patch has the most changes and contains the new helper method, > `clnt_hello_proc smp_client_hello_parse` which does the initial processing of > the client hello message > that was common to the following fetches: > > 1. smp_fetch_req_ssl_st_ext > 2. smp_fetch_req_ssl_ec_ext > 3. smp_fetch_ssl_hello_sni > 4. smp_fetch_ssl_hello_alpn > 5. smp_fetch_ssl_supported_groups > 6. smp_fetch_ssl_sigalgs > 7. smp_fetch_ssl_keyshare_groups > 8. smp_fetch_ssl_cipherlist > > The first 7 fetches do some additional processing based on the TLS extensions > whereas the last one, > `smp_fetch_ssl_cipherlist` does not. Instead it parses the CipherSuite field. > This distinction has been made > in the new helped function by using the boolean parameter `parse_extensions`. > Fetches 1 to 7 will pass true > and `smp_fetch_ssl_cipherlist` will pass `false`. You had mentioned to use a > union for this but I wasn't > sure how to use it here. > > Thank you once again for taking the time to review. I'll make a review of the third patch separately. Thanks! -- William Lallemand
