[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5370

--- Comment #13 from Márton Németh <nm127@xxxxxxxxxxx> 2010-11-23 12:44:27 PST ---
(In reply to comment #10)
ű> pcap-common.c:
> 1) Shouldn't lines 1095 and 1132 read something like the following to account
> for both transfers in or out?:
> 
> if ((phdr->transfer_type & ~URB_TRANSFER_IN) == URB_ISOCHRONOUS) {

We are talking about the "xfer_type" field of the struct mon_bin_hdr of
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/usb/mon/mon_bin.c;h=c436e1e2c3b6274f0fdcdb699ec1fd10a9b2273f;hb=HEAD
. This field is initialized at line 536 and line 632. Both cases this
initialization looks like this:

    ep->xfer_type = xfer_to_pipe[usb_endpoint_type(&urb->ep->desc)];

As you can see this only contains the transfer type, but *not* the direction.
This is handled wrongly in Wireshark several places.

> 2) The iso_numdesc isn't really needed since phdr->s.iso.numdesc can be
> directly used in the for() loop at line 1140 instead.

I tried to aviod using pointer aritmetic inside the "for" loop. That's why I
introduced iso_numdesc. I don't know what code the compiler would generate if
the phdr->... indirection is computed for each iteration in the loop.

> packet-usb.c:
> 1) All multi-byte fields in the USB pseudo header have been byte-swapped, if
> needed, in pcap-common.c so that the values are now in host byte order, i.e.
> the host doing the dissecting.  This means that none of these pseudo header
> fields can be added using proto_tree_add_item(tree, hf_xyz, tvb, offset,
> length, endian), since "endian" can only be ENC_BIG_ENDIAN or
> ENC_LITTLE_ENDIAN.  There is no ENC_HOST_ENDIAN.  So ... where the ISO
> hf_usb_iso_error_count and hf_usb_iso_numdesc fields are added at ~lines
> 2200-2208, this has to instead mimic what dissect_linux_usb_pseudo_header()
> does by doing tvb_memcpy()'s and then calling the proto_tree_add_uint().  This
> applies down around ~lines 2242-2254 as well with hf_usb_iso_status,
> hf_usb_iso_off, hf_usb_iso_len, and even hf_usb_iso_pad.

That's right, this needs to be corrected in the patch.

> 2) I'm confused with this block of code:
>                 if (!iso_status && iso_len && data_base + iso_off + iso_len <=
> tvb_length(tvb))
>                     proto_tree_add_item(tree, hf_usb_iso_data, tvb, data_base +
> iso_off, iso_len, TRUE);
>                 offset += 4;
> 
> You are adding some data in between the iso_len and iso_pad?  Is the iso_pad
> only present if there is no data?  But if that's the case, then why always add
> the iso_pad?  Should the data come after the iso_pad, since the mon_bin_isodesc
> struct is defined as follows?:
> 
> struct mon_bin_isodesc {
>         int          iso_status;
>         unsigned int iso_off;
>         unsigned int iso_len;
>         u32 _pad;
> };

The data is the byte array to where the iso_off points. The data bytes are
starting at iso_off offset and has the length iso_len. The padding is a
different thing here, it is coming from the struct mon_bin_isodesc. There is,
however sometimes padding bytes between the data blocks also, but they are not
handled by this patch. See the example capture file from comment #12.

> 3) For hf_usb_iso_off and hf_usb_iso_len, I think "Offset [bytes]" and "Length
> [bytes]" can just be "Offset" and "Length".  If necessary (which I don't think
> it is), use the blurb field for a more detailed description of the field.

This is following the style of the "URB length [bytes]" and "Data length
[bytes]". I think it is always a good idea to have the unit shown near to the
value. Is this maybe handled differently in other parts of Wireshark?

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.