mdavidsaver / pvxs

PVA protocol client/server library and utilities.
https://mdavidsaver.github.io/pvxs/
Other
19 stars 25 forks source link

fix handling of `pva_ctrl_msg::SetEndian` #27

Closed mdavidsaver closed 2 years ago

mdavidsaver commented 2 years ago

Address my mis-reading of the protocol "spec" (https://github.com/epics-base/pvAccessCPP/issues/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.

AppVeyorBot commented 2 years ago

:white_check_mark: Build pvxs 1.0.784 completed (commit https://github.com/mdavidsaver/pvxs/commit/fe6933e6df by @mdavidsaver)

mdavidsaver commented 2 years ago

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 commented 2 years ago

:white_check_mark: Build pvxs 1.0.786 completed (commit https://github.com/mdavidsaver/pvxs/commit/035062e21d by @mdavidsaver)

mdavidsaver commented 2 years ago

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

kasemir commented 2 years ago

Tried to update https://github.com/epics-base/pvAccessCPP/wiki/Protocol-Messages#set-byte-order-0x02 accordingly