Comment # 10
on bug 8505
from Roland Knall
(In reply to comment #8)
> A few questions and notes:
>
> - read_lowhigh_order appears (at first glance) to simply be doing byte order
> conversion from one endianness to another. We provide the functions
> tvb_get_ntohl and tvb_get_letohl (depending on which endianness you need)
> for precisely this purpose. The calculation of 'ssdoIndex' can similarly be
> replaced with a call to tvb_get_ntohs or tvb_get_letohs.
>
> - In general, duplicating the entire payload with ep_tvb_memdup is
> unnecessary if all you're going to do is fetch certain values from it. There
> should be a function to get any type of value you need directly from the tvb
> without any manual bit-twiddling.
openSAFETY as a protocol applies two features, which in my case, make both
those things a necessity:
- The payload transmitted may be in a different order. oS is a 2-frame
protocol, where the position of frame 2 before or after frame 1 depends on the
implementation for the underlying fieldbus. Therefore, reading directly from
tvb may require some real dramatic hassling with pre- and offsets, as the same
dissection may use different positions in the payload for the same information
- The payload is additionally coded with a CRC in the second sub-frame. This
CRC is decoded, and the correct and unifed payload is then passed on as a byte
stream.
I agree, that this seems to be a little bit too much to pass arround, but the
whole memory is allocated using ep_ functions, and so will be thrown away
anyway at the end of each package.
> - tvb_reported_length is usually preferred over tvb_length in most cases,
> since it permits proper dissection of truncated captures (the result of
> passing -s to tcpdump, for example)
truncated captures cannot be dissected in the case of openSAFETY. Either the
payload is complete or not. The whole payload is CRC secured, and if data is
corrupted, the frame has to be ignored. openSAFETY is a machine-safety (not
security) protocol, and therefore incomplete or incorrect frames MUST be
ignored. But I can change it, if you still prefer the other function.
> - You use mystery constants (such as 0x1018) in several places in the code.
> Naming these with #defines is recommended (I think such names can be used in
> your value strings as well?)
Will do, and send the patch with the new defines. Naming the value strings is
not so much an issue, but will try to use it there as well. Only a few fields
can be dissected in such a way, that mentioning there index and subindex is
possible in code.
> - The ./tools/checkhf.pl script reports one error:
> Unused entry: epan/dissectors/packet-opensafety.c, hf_oss_ssdo_extpar_hdr
>
> - There's one C++-style comment that slipped in accidentally (caught by
> ./tools/checkAPI.pl)
Will do, see next version of the patch
Thanks for the comments
Roland
You are receiving this mail because:
- You are watching all bug changes.