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

cmd/scion: add a ping flag that allows setting the final packet size #4251

Merged
merged 6 commits into from
Sep 12, 2022

Conversation

bunert
Copy link
Contributor

@bunert bunert commented Sep 8, 2022

The new flag packet-size allows specifying the size of the packet, which takes the variable header size into account.
The new flag overwrites the payload-size flag similar to how the max-mtu flag handles it, while the max-mtu flag
now overrides the payload-size and the packet-size flag.


This change is Reviewable

…the final pkg size (independent of headers)

The new flag 'packet-size' allows to specify the size of the actual package which takes the variable header size into account. The new flag overwrite the 'payload-size' flag similar as the 'maxMTU' flag handles it, while the 'maxMTU' flag now overrides the 'payload-size' and the 'packet-size' flag.

Changes:
- additional check if the 'packet-size' is set
- if desired size is smaller than the required overhead for headers, return an error
- 'packet-size' overwrites 'payload-size'
- 'maxMTU' overwrites both
@matzf matzf requested a review from oncilla September 8, 2022 14:02
…the final pkg size (independent of headers)

The new flag 'packet-size' allows to specify the size of the actual package which takes the variable header size into account. The new flag overwrite the 'payload-size' flag similar as the 'maxMTU' flag handles it, while the 'maxMTU' flag now overrides the 'payload-size' and the 'packet-size' flag.

Changes:
- additional check if the 'packet-size' is set
- if desired size is smaller than the required overhead for headers, return an error
- 'packet-size' overwrites 'payload-size'
- 'maxMTU' overwrites both
Copy link
Contributor

@uniquefine uniquefine left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bunert and @oncilla)


scion/cmd/scion/ping.go line 192 at r2 (raw file):

				if overhead > int(flags.pktSize) {
					return serrors.New("desired packet size smaller than header overhead")

I think it makes sense to display the minimum size.

Suggestion:

					return serrors.New("desired packet size smaller than header overhead", "minimum_packet_size", overhead)

scion/cmd/scion/ping.go line 196 at r2 (raw file):

				pldSize = int(flags.pktSize - uint(overhead))

drop this empty line


scion/cmd/scion/ping.go line 276 at r2 (raw file):

		`choose the payload size such that the sent SCION packet including the SCION Header,
SCMP echo header and payload are equal to the MTU of the path. This flag overrides the
'payload_size' and 'packet_size' flag.`)

Suggestion:

flags

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bunert)


-- commits line 2 at r2:
Let's be concise in the title. It should be easy to parse on a glance.
This is especially important if you skim read the git log

Suggestion:

cmd/scion: add a ping flag which allows setting the final packet size

-- commits line 4 at r2:
In networking, the idomatic term is packet, not package.

Suggestion:

packet

-- commits line 4 at r2:
we use kebap-case flags.

Suggestion:

max-mtu

-- commits line 8 at r2:
This does not add any value really. You can drop it.

I think we need to adapt the "good commit message" example in our internal documentation.
https://go.dev/doc/contribute#commit_messages is what we aim for in the end.

Code quote:

  Changes:
  - additional check if the 'packet-size' is set
  - if desired size is smaller than the required overhead for headers, return an error
  - 'packet-size' overwrites 'payload-size'
  - 'maxMTU' overwrites both

Copy link
Contributor Author

@bunert bunert left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @oncilla and @uniquefine)


-- commits line 2 at r2:

Previously, oncilla (Dominik Roos) wrote…

Let's be concise in the title. It should be easy to parse on a glance.
This is especially important if you skim read the git log

Done.


-- commits line 4 at r2:

Previously, oncilla (Dominik Roos) wrote…

we use kebap-case flags.

Done.


-- commits line 4 at r2:

Previously, oncilla (Dominik Roos) wrote…

In networking, the idomatic term is packet, not package.

Done.


-- commits line 8 at r2:

Previously, oncilla (Dominik Roos) wrote…

This does not add any value really. You can drop it.

I think we need to adapt the "good commit message" example in our internal documentation.
https://go.dev/doc/contribute#commit_messages is what we aim for in the end.

Done.


scion/cmd/scion/ping.go line 192 at r2 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

I think it makes sense to display the minimum size.

Done.


scion/cmd/scion/ping.go line 196 at r2 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

drop this empty line

Done.


scion/cmd/scion/ping.go line 276 at r2 (raw file):

		`choose the payload size such that the sent SCION packet including the SCION Header,
SCMP echo header and payload are equal to the MTU of the path. This flag overrides the
'payload_size' and 'packet_size' flag.`)

Done.

Copy link
Contributor

@uniquefine uniquefine left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)


-- commits line 4 at r2:

Previously, bunert wrote…

Done.

The title of the PR is still mentions "package"


-- commits line 4 at r2:

Previously, bunert wrote…

Done.

The PR description still mentions maxMTU

@bunert bunert changed the title cmd/scion: additional scion ping flag for package size cmd/scion: add a ping flag that allows setting the final packet size Sep 12, 2022
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bunert)

@oncilla oncilla enabled auto-merge (squash) September 12, 2022 15:23
Copy link
Contributor

@uniquefine uniquefine left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bunert)

Copy link
Contributor Author

@bunert bunert left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bunert)


-- commits line 4 at r2:

Previously, uniquefine (Sebastian Leisinger) wrote…

The title of the PR is still mentions "package"

Fixed it. But a question: should the PR title include the first line (https://go.dev/doc/contribute#commit_messages) or is this just about the PR description while the title is just for the PR?


-- commits line 4 at r2:

Previously, uniquefine (Sebastian Leisinger) wrote…

The PR description still mentions maxMTU

Should be correct now.

@oncilla oncilla merged commit d644ecf into scionproto:master Sep 12, 2022
@bunert bunert deleted the bunert/issue10736 branch September 15, 2022 11:31
benthor pushed a commit to benthor/scion that referenced this pull request Nov 24, 2022
…cionproto#4251)

The new flag `packet-size` allows specifying the size of the packet, which takes the variable header size into account.
The new flag overwrites the `payload-size` flag similar to how the `max-mtu` flag handles it, while the `max-mtu` flag
now overrides the `payload-size` and the `packet-size` flag.
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