Skip to content

Consolidation and Enhancement of BSD Socket Layer#4660

Closed
KaganCanSit wants to merge 1 commit into
randombit:masterfrom
KaganCanSit:feature
Closed

Consolidation and Enhancement of BSD Socket Layer#4660
KaganCanSit wants to merge 1 commit into
randombit:masterfrom
KaganCanSit:feature

Conversation

@KaganCanSit

@KaganCanSit KaganCanSit commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

Hello Botan Development Team,

In reference to Issue #2216, I have undertaken the consolidation and enhancement of the BSD socket layer. This endeavor involved refactoring redundant code within socket.cpp, socket_udp.cpp, and cli/socket_utils.h to foster a more streamlined and maintainable architecture.

Post-refactoring, the codebase compiles successfully, and the botan-test suite executes without errors. However, I am encountering challenges in testing the tls-server and tls-client applications. I would greatly appreciate guidance and assistance in effectively conducting these tests.

As a newcomer to the Botan project, I am impressed by its adherence to high C++ standards, utilization of CMake, and comprehensive documentation via Doxygen. I am eager to contribute to both my personal development and the project's advancement.

I recognize that further simplification could be applied to the files in question. Nonetheless, I am aware that extensive modifications can complicate analysis and review processes. Following this PR, I am open to implementing additional refinements based on your guidance.

I acknowledge that there may be errors in my contributions and am open to making necessary changes. I welcome any feedback and am willing to collaborate to ensure the highest quality of code.

Note: I checked the necessary files with clang-format to ensure that they comply with the file standards. However, despite updating to the latest version, I still received warnings on Debian 12 about "RequiresClausePositin and IndentRequiresClause", so I removed them.

Thank you for considering my contribution.

Sincerely

@KaganCanSit
KaganCanSit marked this pull request as draft February 9, 2025 17:44
@reneme

reneme commented Feb 11, 2025

Copy link
Copy Markdown
Collaborator

Hi Kağan! Thanks a lot for looking into this. I didn't look into your code so far, just a quick reply to:

I am encountering challenges in testing the tls-server and tls-client applications. I would greatly appreciate guidance and assistance in effectively conducting these tests.

You can use those CLI-tools to quickly set up and test a TLS client and/or server. It makes sense to test those extensively, as they make use of the socket abstraction API, as you know.

The easiest way to test them is via the CLI-tests. For that, just build the library as usual and enable the CLI. Eg. like that:

 ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli --build-tool=ninja
ninja cli

The CLI-tests are implemented as a bunch of python functions in src/scripts/test_cli.py. To run the TLS-client/server specific one just go for:

python3 src/scripts/test_cli.py ./botan cli_tls_socket_tests

This runs several tests with a Botan tls_client connecting to a Botan tls_server with different protocol configurations. This should be enough to effectively test those CLI tools.

@KaganCanSit

KaganCanSit commented Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

Thanks, @reneme. Your guidance was extremely helpful.

I reviewed and updated my ninja and ccache tools:

sudo apt-get upgrade ninja-build
sudo apt-get upgrade ccache

Then, after compiling the project, I ran the command you shared:
python3 src/scripts/test_cli.py ./botan cli_tls_socket_tests

The result seems to be successful:

INFO: Ran cli_tls_socket_tests in 0.91 sec
5 tests were run in 0.90 seconds with 0 failures

I would like to state that I am open to doing additional testing and improvements. After reviewing the code content, we can refactor the code for readability if necessary. I am open to additional feedback.

Thanks again for your time and support.

Note: I also want to add. There are still repetitive structures in these sections. But I know that major edits will not be accepted. If this PR goes well and I embrace the scope a little more, we can make another simplification with your guidance.


I have found the time to look at this problem again. I see that 19 out of 38 tests failed, but the Nightly build completed successfully. I will look for errors and try to fix them. Then if you specify any changes you want to make to the parts we reviewed, I will try to make them.

First I look this -> InternalError: No header guard start found in ./src/lib/utils/socket/socket_platform.h

@KaganCanSit
KaganCanSit force-pushed the feature branch 2 times, most recently from e05cb39 to a27b653 Compare February 18, 2025 17:04
@coveralls

coveralls commented Feb 18, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 90.648% (-0.004%) from 90.652%
when pulling 1b04770 on KaganCanSit:feature
into a2a0b43 on randombit:master.

Comment thread src/lib/utils/socket/socket_platform.h
@KaganCanSit
KaganCanSit force-pushed the feature branch 5 times, most recently from 7f9260a to b720e7b Compare February 20, 2025 06:22
@KaganCanSit
KaganCanSit marked this pull request as ready for review February 20, 2025 07:13
@KaganCanSit

This comment was marked as outdated.

@KaganCanSit
KaganCanSit requested a review from reneme February 20, 2025 07:14
reneme

This comment was marked as resolved.

@KaganCanSit
KaganCanSit force-pushed the feature branch 2 times, most recently from d216790 to c53c16e Compare March 3, 2025 20:40
@KaganCanSit
KaganCanSit requested a review from reneme March 4, 2025 04:54
Comment thread src/cli/tls_client.cpp Outdated
@KaganCanSit
KaganCanSit force-pushed the feature branch 5 times, most recently from 8c37e42 to 14cedad Compare March 4, 2025 20:56
@KaganCanSit
KaganCanSit requested a review from reneme March 5, 2025 05:11

@reneme reneme left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like a decent improvement to me now. Thanks a lot! I'd nevertheless appreciate @randombit to have a final look at this. From my side: LGTM.

@KaganCanSit

Copy link
Copy Markdown
Contributor Author

Thank you for sharing your knowledge with me and guiding me. I have gained new knowledge. @randombit can be merged after checking. I am just a message away if new edits are needed.

Good day and see you for new merge requests.

@reneme
reneme requested a review from randombit March 27, 2025 08:43
@KaganCanSit
KaganCanSit force-pushed the feature branch 3 times, most recently from dff0bc5 to 1f97ef2 Compare June 22, 2025 20:26
@KaganCanSit
KaganCanSit force-pushed the feature branch 5 times, most recently from ace51f3 to 5159109 Compare August 8, 2025 21:48
@KaganCanSit

Copy link
Copy Markdown
Contributor Author

It has been over a year since these improvements were made. Most of the files contain changes, however minor. I believe this improvement needs to be revisited comprehensively, so for now I am closing this in favor of #5422.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants