Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 9517] New dissector for the CCSDS CFDP protocol

[bugzilla-daemon@xxxxxxxxxxxxx]

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.