Wireshark-bugs: [Wireshark-bugs] [Bug 8859] Apply consistent naming convention to checksum displ
Comment # 3
on bug 8859
from Michael Mann
(In reply to comment #2)
> Creating a proto_tree_add_checksum seems a good idea, but it may get hairy.
> First some semantics, right now we have some fields:
> PROTO.checksum
> PROTO.checksum_bad
> PROTO.checksum_good
> And recently I have been adding:
> PROTO.checksum_calculated.
> checksum_bad and checksum_good are oddballs. I
> would expect them to act as a boolean for existence, but you actually have
> to check ip.checksum_bad==1 for example. If checksumming is enabled, then
> PROTO.checksum_bad==1 implies PROTO.checksum_good==1 (and vice versa). Why
> do we have two fields for this?
I think PROTO.checksum_good=1 is technically not valid if checksuming is
disabled (but it's not "bad" either, which is why there are 2 fields)
> Some dissectors add expert info for bad
> checksums (shouldn't this also be included in the generic function?), such
> as tcp.checksum_bad.expert.
I think an expert info should be included in the proto_tree_add_checksum
function.
> Initial prototype I thought of before
> considering other problems:
proto_tree *
> proto_tree_add_checksum_tree(proto_tree *tree, int hfindex, tvbuff_t *tvb,
> gint start, gint length, const char *cksum_expected);
I was thinking something closer to this:
proto_item*
proto_tree_add_checksum(proto_tree *tree, checksum_t* checksum_hfs, tvbuff_t
*tvb, gint start, gint length, guint32 (64?) chsum_expected);
with checksum_t being a structure that contains all of the necessary hfs
(checksum, good, bad, expected?, ett_ for "checksum tree", etc) and
expert_info. It beats having to pass that many parameters into the function.
Having a guint32 for checksum should cover most dissectors. We may not get ALL
dissectors with the new API, but I think this casts as wide a net as possible
(and note many dissectors may need to add a few more hfs_ in order to use this
API, but the consistency is desired).
(off-topic)
> Btw, I see a "fp.header.bad_checksum." field with a trailing dot. Shouldn't
> that be disallowed? (dissector assert / checkAPI.pl?)
Yea, that's a typo. If "fp.header.bad_checksum" already exists, then the
expert info should be "fp.header.bad_checksum.expert". If it doesn't already
exist, then the trailing period can be removed.
You are receiving this mail because:
- You are watching all bug changes.
