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

Advanced split options #309

Merged

Conversation

PeterZhizhin
Copy link
Contributor

Allow split to work in a similar way to byeDPI

Copy link
Contributor

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions!

// using [SplitWriter].
func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int64) (transport.StreamDialer, error) {
// using [SplitWriter]. If "repeatsNumber" is not 0, will split that many times, skipping "skipBytes" in between packets.
func NewStreamDialer(dialer transport.StreamDialer, prefixBytes int64, repeatsNumber int64, skipBytes int64) (transport.StreamDialer, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make splitDialer public (SplitDialer) and return that in this function, and add setters for the repeat and skip. Keep the member variables private so people don't mess with them.

As a design philosophy, we would rather keep the constructor with only the required parameters, with optional options as setters. That keeps the code extensible and simple for those that don't care about the advanced options.

This approach will preserve backwards compatibility as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aargh, the setters don't work as proposed beecause we return different implementations depending on the parameters. Need to rethink...

@@ -19,8 +19,10 @@ import (
)

type splitWriter struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note as for the Dialer. Let's make this public and return the implementation in the NewWriter call, with setters for the optional parameters.

x/go.mod Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

@@ -78,7 +78,7 @@ func main() {
methodFlag := flag.String("method", "GET", "The HTTP method to use")
var headersFlag stringArrayFlagValue
flag.Var(&headersFlag, "H", "Raw HTTP Header line to add. It must not end in \\r\\n")
timeoutSecFlag := flag.Int("timeout", 5, "Timeout in seconds")
timeoutSecFlag := flag.Int("timeout", 100500, "Timeout in seconds")
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

prefixBytes int64
writer io.Writer
prefixBytes int64
repeatsNumberLeft int64 // How many times left to split
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move these comments to the public setters.

if rf, ok := writer.(io.ReaderFrom); ok {
return &splitWriterReaderFrom{sw, rf}
// If repeatsNumber > 1, then packets will be split multiple times, skipping skipBytes in between splits.
// Example:
Copy link
Contributor

Choose a reason for hiding this comment

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

So the slice points will be in positions prefixBytes + i * skipBytes, for 0 <= i < repeatsNumber.

It may be worth it writing down that formula. It clarifies things.

This formula made me realize that skipBytes and repeatsNumber are not meaningful by themselves, and must always be specified together.

So perhaps we need a setter like SplitWriter.SetMultiSplit(splitBytes, splitCount).
It could be other names, like Enable instead of Set, or Additional, Extra instead of Multi, ... Open to ideas.

I think splitCount in this case should be for the extra blocks, in order to not be confusing. In that case the formula would be:
prefixBytes + i * splitBytes, for 0 <= i <= splitCount. With the default being splitCount == 0.

@@ -38,7 +38,7 @@ func (w *collectWrites) Write(data []byte) (int, error) {

func TestWrite_Split(t *testing.T) {
var innerWriter collectWrites
splitWriter := NewWriter(&innerWriter, 3)
splitWriter := NewWriter(&innerWriter, 3, 0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

These can all be reverted with the backwards-compatible API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update configurl/doc.go with the new format.


func parseURL(splitUrl string) (prefixBytes int64, repeatsNumber int64, skipBytes int64, err error) {
// Split the input string by commas
parts := strings.Split(splitUrl, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a comma-separated value, let's use proper URL variables.
It would read something like prefix=1&count=2&length=10. Need to figure out proper names.

You can fallback to variables if it fails to parse as a number.

@fortuna fortuna requested a review from jyyi1 November 3, 2024 20:35
@fortuna fortuna marked this pull request as ready for review November 4, 2024 22:52
@fortuna fortuna changed the base branch from main to fortuna-split November 4, 2024 22:53
Copy link
Contributor

Choose a reason for hiding this comment

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

Damn, I tried to revert these files on github and ended up removing them by accident. Still trying to figure out this cross-repo thing

@fortuna
Copy link
Contributor

fortuna commented Nov 4, 2024

Merging to make changes

@fortuna fortuna merged commit f718f69 into Jigsaw-Code:fortuna-split Nov 4, 2024
1 check passed
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.

2 participants