Wireshark-bugs: [Wireshark-bugs] [Bug 8505] openSAFETY: New reassemble functionality, as well as
Comment # 11
on bug 8505
from Evan Huus
(In reply to comment #10)
> (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
I'm not sure how this is relevant? You'll either have to calculate offsets into
the copied buffer or offsets into the tvb, but they'll be the same offsets
either way.
> - 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.
Coded as in checksummed? Then validating the checksum shouldn't change the
payload. I'm not clear on how you'd use a CRC in any other way...
> 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.
Leaks are not the concern, I just thought the entire thing could be simplified.
> > - 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.
There is a difference between "the spec says this frame must be discarded" and
"wireshark cannot dissect this frame". Wireshark dissects a lot of 'bad' frames
because that's arguably the most useful part of the stream when debugging a
network protocol. They should be marked with expert info where possible, so
that they can be distinguished from good frames in the UI, but we still want to
provide as much decoding as we can.
Again, it sounds like there's a CRC encrypting the entire frame, but this
doesn't make sense to me, since CRCs are for checksumming only?
> > - 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.
