-
Notifications
You must be signed in to change notification settings - Fork 30
document how proxy_{get,set}_property expects paths to be serialized #94
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Warres <mpw@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @kyessenov
abi-versions/v0.2.1/README.md
Outdated
and then concatenated in sequence to form the `path_data` argument. | ||
|
||
e.g. the path segments `["foo", "bar"]` would be serialized as: | ||
- `0x66`, `0x6f`, `0x6f`, `0x00`, `0x62`, `0x61`, `0x72`, `0x00` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not entirely correct. There is no NULL
character at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, while you're right about the Rust and Go SDKs not including a NULL
char at the end, the C++ SDK does: code.
I can fix the C++ SDK to trim the last NULL
char, but I think that host implementations would still need to tolerate a terminating NULL
char in order to work with C++ plugins compiled prior to this fix. The Envoy implementation of getProperty
does appear to tolerate this.
ISTM that in order to document the acceptable range of behaviors, the spec should say that path_data
can (but is not required to) contain a terminating NULL
char. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to state that null char is a separator, but host implementations should also tolerate it as a terminator if present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The segments could be empty, so the distinction is important actually. Although I don't recall if we ever tested it with empty segments, so it's under-specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reading of this logic in Envoy::Extensions::Common::Wasm::getProperty()
is that:
- It handles each segment by finding the next null char (if any), and taking the substring up until that (which could be empty)
- It bumps
start
to the previousend
(which doesn't include null char) + 1 (skipping over the null char, if present) - Loop terminates when
start >= path.size()
. In the case of apath
that does include a terminating null char,start
will end up== path.size()
, while in the case of apath
that doesn't,start
will end up equal topath.size() + 1
. Either way, the loop terminates.
So I think it's still accurate to say that null char is a separator (which doesn't rule out empty segments), and that an optional terminating null char should be tolerated by host implementations.
If there's a different wording that either of you would prefer, I'm open to suggestions. Thanks!
But also stipulate that host implementations must tolerate a terminating null char if present. Signed-off-by: Michael Warres <mpw@google.com>
Document the de-facto serialization format used by the various proxy-wasm SDKs (C++, Rust, Go) to serialize
path_data
arguments for theproxy_get_property
andproxy_set_property
hostcalls.