Hello, On Thu, 27 Jun 2019 at 08:43, Илья Шипицин <[email protected]> wrote: >> However this commit also changes error handling with OpenSSL: >> >> https://github.com/haproxy/haproxy/commit/54832b97c65c43ee50b6661a9b4b05885427b5e9#diff-cc9b3f376afbc81cf478eeac17839172L5533 >> >> >> Can you clarify if the OpenSSL change was intentional and if yes, elaborate? > > > it was intentional. libressl does not support "packet_size" there. > however, openssl is fine with the above way
Then I have to criticize that commit message. It fixes the build, but it changes a function call, so it's more than just a build fix. It's also not affecting only the LibreSSL code path, but the OpenSSL code path also. A single commit should do one thing only (as opposed to a number of things) and reflect the changes it does in both title and message. "BUILD: enable several LibreSSL hacks, including" when touching OpenSSL code paths is not enough. This should have been 1) a separate commit and 2) have a descriptive message like: "ssl: change error handling on empty handshake Also fixes the build with LibreSSL" Because that's what the change does: a different error handling for empty handshakes, which fixes the build with LibreSSL but the change affects (affects in that in changes the code) OpenSSL as well. The change breaks empty handshake detection in OpenSSL < 1.1.0. We need to revert this and put the LibreSSL fix in a different #if branch. > however, openssl is fine with the above way That assumption is wrong, but that's not the point. The point is that we need to do a better job of making one change only per commit and have title and commit message reflect the actual change and intention, not the assumed impact. thanks, lukas
