Bill Meier
changed
bug 9517
| What |
Removed |
Added |
| Status |
IN_PROGRESS
|
INCOMPLETE
|
Comment # 1
on bug 9517
from Bill Meier
Somme comments:
1. col_set...(), col_add...() should not be called under 'if (tree)'
(IOW: the 'if (tree)' will need to be removed (or used only around code
which just updates the packet dissection display (proto_tree_dd_item &
etc)).
This also applies to one dissector calling another. So the 'if (tree)'
should also be removed in packet-ccsds.c
2. '//' is not handled by all the compilers we support. If you want to comment
out some lines of code, you should use '#if 0/#endif'
3. There are many lines with trailing whitespace; please remove same.
4. There are hf[] entries throughout the code which
specify the 'field_type' as FT_UINT16 while really they are
being used to display a value
which is either a single full byte or is contained within a single byte.
In these cases, I think that the field_type should be FT_UINT8 with
a bit mask only if the field is less than a full byte.
For bit fields, the field-type determines the width of the display for the
value;
For bit fields with a bit mask and using FT_UINT16,
you'll get 16 bits displayed:
.... .... 0... .... (for example).
I suspect this is not really what you want.
In general, if you are displaying a singe byte (or a field within a
single byte), use type FT_UINT8 and specify a bit mask only if the
field is smaller than a single byte.
5. Re: dissect_cfdp_src_entity_id()
Many of the hf[] entries used in this function use bit masks which
will result in a display which might not be as you wish;
that is: you'll get a display like:
.... 0000 1111 2222 ...
I suggest doing the following:
Define *one* hf[] hf_cfdp_srcid entry with type FT_UINT64,
display BASE DEC and no bitmask.
Then dissect_cfdp_src_entity_id(...) becomes
dissect_cfdp_src_entity_id(...) {
if (len_ent_id != 0 && len_ent_id <= 8) {
proto_tree_add_item(tree, hf_cfdp_srcid_1, tvb, offset,
len_ent_id, ENC_BIG_ENDIAN);
} else {
... wrong length ...
}
}
This is a bit of a hack, but it seems to work as long as you are
using BASE_DEC.
The number of bytes fetched from the tvb buffer will be determined by
the length specified in the proto_tree_add_item() args.
Note that this hack doesn't work well if you are using BASE_HEX.
(All the values will be displayed as 0xnnnnnnnnnnnnnnnn).
(Someone will correct me if I'm missing something in the above suggestion).
This comment also applies to:
dissect_cfdp_tseq_num()
dissect_cfdp_dst_entity_id()
(and others ?)
That's all I have time for ...
--------
You've spent some time and effort on this which is appreciated. After you've
addressed the above comments, I (or someone else) will be glad to do a further
review.
Please feel free to ask any questions you may have about the Wireshark API.
Bill
You are receiving this mail because:
- You are watching all bug changes.
