[bugzilla-daemon@xxxxxxxxxxxxx]

Bill Meier changed bug 9324

What	Removed	Added
Status	IN_PROGRESS	INCOMPLETE

Comment # 3 on bug 9324 from Bill Meier

Notes:

1. It appears that almost *all* of the code under dissect_tfp_tcp()
   and dissect_tfp_usb() is identical. Please use a common subroutine
   instead of duplicating code;

   Also: re: dissect_tfp_usb()
     if((usb_conv_info->deviceVendor == tfp_USB_VENDOR_ID) &&
        (usb_conv_info->deviceProduct == tfp_USB_PRODUCT_ID))

     There's no action of the if test fails.

     I suspect that the usb dissection needs to be updated to properly
     handle IF_CLASS_VENDOR_SPECIFIC (maybe by allowing registration
     of vendor specific dissectors in a usb vendor specific table).
     That way, the tfb dissector would be called only for tfp_USB_VENDOR_ID.
     (I'll poke wireshark-dev separately to see how best to  handle
      that issue. The tfp dissector can be updated once the usb dissector
      changes are done).

   For now: let's just do something like the following:

       dissect_tfp_tcp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
       {
           col_set_str(COL_PROTO, "TFP over TCP");
           dissect_tfp_common(tvb, pinfo, tree);
       }


       dissect_tfp_usb(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
       {

       usb_conv_info_t *usb_conv_info;
       usb_conv_info = (usb_conv_info_t *)pinfo->usb_conv_info;

       if(usb_conv_info->deviceVendor == tfp_USB_VENDOR_ID)
           {
               col_set_str(COL_PROTO, "TFP over USB");

               if (usb_conv_info->deviceProduct == tfp_USB_PRODUCT_ID)
               {
                   dissect_tfp_common(tvb, pinfo, tree);
               } else
               {
                   col_set_str(COL_INFO, "Unknown/Invalid Product...");
                   call_dissector(data_handle, tvb, pinfo, tree);
               }
           } else
           {
               return;  /* not tfp_VENDOR_ID: punt */
           }
       }


2. Most of "bit & byte offsets" static vars appear to actually be used
   as constants which each might better be 'static const' or #define e.g.,
   static const gint byte_count_tfp_uid = 4"

3. inttypes.h is part of C99. It not necessarily the case that a C
     compiler supported by Wireshark supports C99.
   so: please use guint32 instead of uint32_t & etc (and don't include
     inttypes.h)

4. There are some unnecessary casts;
   e.g.,
    base58_encode((uint32_t)tvb_get_letohl(tvb, 0), &tfp_uid_string[0]);

5. global vars should be local (auto) vars.

   static proto_tree *tfp_tree = NULL;

   static gint byte_offset = 0;
   static gint bit_offset = 48;

   static guint8 hv_tfp_len = 0;
   static guint8 hv_tfp_fid = 0;
   static guint8 hv_tfp_seq = 0;

6. Please remove all trailing whitespace

7. There's no need to create vars for the following:
     static const char *tfp_proto_name_tcp = "TFP over TCP";
     static const char *tfp_proto_name_usb = "TFP over USB";
   Just specify the string as a literal in the call to col_set_str()
     (or use a #define). (See #1 above).

8. COL_PROTO & COL_INFO; sequence of calls;
     Best sequence:

      col_set_str(COL_PROTO, ...)
      col_clear(COL_INFO, ...);

      base58_encode((uint32_t)tvb_get_letohl(tvb, 0), &tfp_uid_string[0]);
      hv_tfp_len = tvb_get_guint8(tvb, tfp_BYTE_OFFSET_LEN);
      hv_tfp_fid = tvb_get_guint8(tvb, tfp_BYTE_OFFSET_FID);
      hv_tfp_seq = tvb_get_bits8( tvb, tfp_BIT_OFFSET_SEQ,
                                       bit_count_tfp_seq);

      col_add_fstr(pinfo->cinfo, COL_INFO, ...);

      See doc/README.dissector: Section 1.4.4 The col_clear function.
      Basically: the column initialization should be done before
                fetching from the tvbuff so that the COLS will contain
                something appropriate even if one of the calls to
                tvb_get..() throws an exception (e.g., because there's
                not enough data in the tvbuff).

9. Use tvb_reported_length() rather than tvb_length().
   'reported length' is the actual length of the packet on the wire.
   'length' is is the number of bytes captured (which may be less than
   the reported length' if the capture was done with a limited
   'snapshot length'

---

      You are receiving this mail because:

• You are watching all bug changes.