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

Default for Control checksum coverage #315

Closed
gorryfair opened this issue Apr 29, 2019 · 8 comments
Closed

Default for Control checksum coverage #315

gorryfair opened this issue Apr 29, 2019 · 8 comments
Assignees

Comments

@gorryfair
Copy link
Contributor

In Section 5.2.7 Control checksum coverage on sending

“The recommended default is to Ignore this option.”

I argue that this default is much braver than specified in the IETF present in RFC 8085. The default for IPv6 is also that this is required by default. Again, I would argue that applications need to explicitly be designed to accept this.


In Section, 5.2.8. Control checksum coverage on receiving

“The recommended default is to Ignore this option.”

I argue that this default is much braver than specified in the IETF present in RFC 8085. The default for IPv6 is also that this is required by default. Again, I would argue that applications need to explicitly be designed to accept this.

@gorryfair gorryfair added the API label Apr 29, 2019
@britram
Copy link
Contributor

britram commented May 6, 2019

this seems reasonable to me, I don't recall how we ended up at Ignore.

@britram britram self-assigned this May 6, 2019
@philsbln
Copy link
Contributor

philsbln commented May 6, 2019

@gorryfair I think we need to re-phrase this as I guess you mis-read it.

The property only defines whether you want a protocol that provides this feature, not whether it should be enabled. The latter is controlled by the property in 7.3.6.

@abrunstrom
Copy link
Contributor

Yes, this seems to be a misunderstanding. Perhaps we also need to clarify that Ignore just means that the property is ignored when ranking alternatives for candiadet selection and racing, not that the application can ignore it?

@gorryfair
Copy link
Contributor Author

I don't understand something about 5.2.7 and 5.2.8 ... So let me ask a more stupid question:

  • IETF transports always do provide full coverage by default.
  • Why do we have a property to request control of checksum coverage at all, why don't we just have a property that says you want to have (require) or would like to have (prefer), or don't care (ignore) some size of limited checksum coverage - as in 7.3.6.

If you don't get the coverage that you ask for, then you will get more coverage. ... so "require" is in some way "prefer a protocol that gives at least an ability to provide this much?".


Then in 7.3.6:

I think " This property specifies the length of the section of the Message...." is actually the minimum length of message - the transport can always protect more.

I don't understand: " ... a special value (e.g. -1) can be used to indicate the default. "

  • the default is full coverage, how is this different to a zero parameter?

Gorry

@britram
Copy link
Contributor

britram commented May 8, 2019

per TAPS interim, let's Avoid this.

@mwelzl
Copy link
Contributor

mwelzl commented May 24, 2019

PR #330 partially addresses this:
7.3.6 was re-written to include "minimum" and fix the special value related text, also using the same style as in the other Message Properties now to specify the default.

@mwelzl
Copy link
Contributor

mwelzl commented Jul 2, 2019

PR #335 addresses the last missing bit here:

IETF transports always do provide full coverage by default.
Why do we have a property to request control of checksum coverage at all, why don't we just have a property that says you want to have (require) or would like to have (prefer), or don't care (ignore) some size of limited checksum coverage - as in 7.3.6.

I changed both Selection Properties to ask for full coverage and have them default to Require. The effect is similar to before, but intuitively this is clearer and makes more sense, I suppose.

britram added a commit that referenced this issue Jul 3, 2019
@britram
Copy link
Contributor

britram commented Jul 3, 2019

fixed by #335

@britram britram closed this as completed Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants