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

fix handling of pva_ctrl_msg::SetEndian #27

Merged
merged 1 commit into from
May 20, 2022
Merged

fix handling of pva_ctrl_msg::SetEndian #27

merged 1 commit into from
May 20, 2022

Conversation

mdavidsaver
Copy link
Member

Address my mis-reading of the protocol "spec" (epics-base/pvAccessCPP#183) by respecting pva_flags::MSB only when pva_ctrl_msg::SetEndian is received. However, still not inspecting the size field for pva_ctrl_msg::SetEndian. This brings PVXS in line with pvAccessCPP.

@mdavidsaver mdavidsaver added the bug Something isn't working label May 18, 2022
@mdavidsaver mdavidsaver self-assigned this May 18, 2022
@AppVeyorBot
Copy link

Build pvxs 1.0.784 completed (commit fe6933e6df by @mdavidsaver)

* So we latch the byte order here, as the peer will ignore the MSB
* flag in our messages...
*/
sendBE = header[2]&pva_flags::MSB;
Copy link

Choose a reason for hiding this comment

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

The new java implementation behaves like this:
The 'size' field 0x00000000 or 0xffffffff is indeed ignored.
The MSB flag is used to set the equivalent of the sendBE flag, and future messages that the client sends will always use the endianess that the server configured via the set-endian command. For received messages, however, we continue to check the byte order flag in each received messages,

This specific PVXS change is only about the sendBE, locking the byte order of messages that the client sends to the server from now on, making 0x00000000 the default and only option.
What about received messages? Should the set-endian command also affect received messages, we trust the server to from now on only give us messages in that same byte order, or should the client still check the byte order flag of each received message?

@mdavidsaver
Copy link
Member Author

I've confirmed both the issue and this fix with pvAccessCPP running on (simulated) powerpc.

What about received messages?

Unlike pvAccessCPP/Java, PVXS will continue to decode using the MSB flag in each header.

https://github.com/mdavidsaver/pvxs/blob/d717ecd9edd0ab7cc7a652184e41fd7c2bec77c4/src/conn.cpp#L131-L134

... we trust the server to from now on only give us messages in that same byte order, or should the client still check the byte order flag of each received message?

imo. The "spec" should be that the MSB flag in each message header must reflect how that message body is encoded. Otherwise eg. PVA decoding in wireshark couldn't work.

The fact that pvAccessCPP/Java server will accept inconsistency by clients on TCP is something which I see as an implementation specific quirk.

I'm not proposing to add a specific warning or error check to PVXS. I'm reasonably confident PVXS would handle this as an explicit decode error or timeout (eg. if a message size is encode in reverse order).

@AppVeyorBot
Copy link

Build pvxs 1.0.786 completed (commit 035062e21d by @mdavidsaver)

@mdavidsaver mdavidsaver merged commit cce7972 into master May 20, 2022
@mdavidsaver
Copy link
Member Author

Merged. I also added ec8d0df with a test of sending with both LSB and MSB.

@kasemir
Copy link

kasemir commented May 24, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants