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.
