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.

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

Implement ODELAY option #491

Merged
merged 13 commits into from
Feb 28, 2025
Merged

Implement ODELAY option #491

merged 13 commits into from
Feb 28, 2025

Conversation

iamsb97
Copy link
Contributor

@iamsb97 iamsb97 commented Feb 19, 2025

Issue #80.

Approach

Used an atomic variable to store default option and exposed two functions (setter and getter) to modify/retrieve the options. This allows initializing the target with certain options as ODELAY requires it be passed before target is created.

Benefits to this approach:

  1. Ensures backward compatibility as nothing changes on the current user side behavior.
  2. Initiates all subsequent targets with the same default options which helps avoid setting options for each target individually. It can still be overridden ofcourse.

Targets covered:

  1. File
  2. Network
  3. Socket

Targets pending:

  1. WEL
  2. SQLite3

Targets not applicable:

  1. Buffer
  2. Stream
  3. Journald
  4. Chain

@iamsb97 iamsb97 changed the title Issue 80 Implement ODELAY option Feb 19, 2025
@goatshriek goatshriek self-assigned this Feb 19, 2025
@goatshriek goatshriek added the enhancement new features or improvements label Feb 19, 2025
Copy link
Owner

@goatshriek goatshriek left a comment

Choose a reason for hiding this comment

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

This is off to a good start! I think scoping it to a few targets for the first implementation is a good way to start.

Rather than performing the checks and open calls in the sendto functions, I would prefer to see support added to stumpless_open_target and stumpless_target_is_open for these target types. stumpless_add_entry can then check the options on the target and these functions to see if ODELAY is set and attempt to open the target if appropriate.

This will allow users to have finer-grained control over precisely when a target is opened, using the same functionality that would need to be implemented anyway. If they don't want to tune this functionality, then it will happen automatically in stumpless_add_entry and they won't need to do anything other than set the option.

It'll also keep you from needing to make too many changes to the semantics of the sendto functions, like changing parameters, who holds what locks, etc.

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 89.89899% with 10 lines in your changes missing coverage. Please review.

Project coverage is 90.62%. Comparing base (27aae29) to head (d51f270).
Report is 1 commits behind head on latest.

Files with missing lines Patch % Lines
src/target.c 86.27% 5 Missing and 2 partials ⚠️
src/target/socket.c 90.62% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #491      +/-   ##
==========================================
- Coverage   90.63%   90.62%   -0.02%     
==========================================
  Files          47       47              
  Lines        4433     4501      +68     
  Branches      592      601       +9     
==========================================
+ Hits         4018     4079      +61     
- Misses        272      277       +5     
- Partials      143      145       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iamsb97
Copy link
Contributor Author

iamsb97 commented Feb 21, 2025

@goatshriek : I think I have fixed all the other errors except windows build. There aren't any details in workflow logs to understand what is going wrong. Can you help me with how to proceed here?

@goatshriek
Copy link
Owner

From the build log it looks like the socket include added to src/target.c is causing an issue as the sys/socket.h is not available in Windows. The best way around this is probably to add some config wrappers to include/private/config/wrapper/socket.h for is open and open, similar to what the network targets use. The definitions of those functions would go in src/target/socket.c and be able to use the include with no issue, and the wrapper will prevent inclusion when support is not available.

@iamsb97
Copy link
Contributor Author

iamsb97 commented Feb 21, 2025

Thanks for your help and patience with this PR! I believe all workflow checks should pass now (maybe except coverage).

@iamsb97 iamsb97 marked this pull request as ready for review February 26, 2025 12:55
Copy link
Owner

@goatshriek goatshriek left a comment

Choose a reason for hiding this comment

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

Great work on this! There are a number of tweaks outlined below, after which this should be good to merge.

@iamsb97
Copy link
Contributor Author

iamsb97 commented Feb 27, 2025

The line which I removed in this commit was added previously when I was closing the target on failure. Since close_target was removed, this assertion should have failed everytime but somehow it did not fail in linux and windows checks which seems a little strange to me.

@goatshriek
Copy link
Owner

I'm not completely sure why that assertion was passing, as you're correct that it shouldn't. I will make a separate point to go look into that.

It does raise a point that probably needs clarification in the documentation though - that the current target will be set by an open call even if ODELAY is used on that target. As a final change to this, can you clarify this point in the documentation for the current target, which is the documentation for stumpless_get_current_target in include/stumpless/target.h. Adding a sentence to the block describing what the current target is should be enough.

@goatshriek goatshriek merged commit 940d885 into goatshriek:latest Feb 28, 2025
54 of 56 checks passed
@goatshriek
Copy link
Owner

Thanks again for putting this together and sticking with the update through the requested changes! Excellent work.

@iamsb97 iamsb97 deleted the issue-80 branch March 1, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants