[bugzilla-daemon@xxxxxxxxxxxxx]

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

Michal Labedzki <michal.labedzki@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9019|0                           |1
        is obsolete|                            |
   Attachment #9019|review_for_checkin?         |
              Flags|                            |
   Attachment #9020|0                           |1
        is obsolete|                            |
   Attachment #9106|                            |review_for_checkin?
              Flags|                            |

--- Comment #12 from Michal Labedzki <michal.labedzki@xxxxxxxxx> 2012-09-10 02:47:26 PDT ---
Created attachment 9106
  --> https://bugs.wireshark.org/bugzilla/attachment.cgi?id=9106
[PATCH] Bluetooth: Add AVCTP dissector

New version of patch. See below. Also bump AVCTP version to 1.4.

(In reply to comment #7)
> (In reply to comment #0)
> > Created attachment 9019 [details]
> > [PATCH] Bluetooth: Add AVCTP dissector
>
> 1. Can you use the fragment reassembly API from reassembly.[ch]?
> 2. While I've been trying to rid the dissectors of (unnecessary) g_ alloced
> memory, unfortunately for fragmentation it makes sense because otherwise you're
> stuck with twice the memory necessary (fragmented and then as a reassembled
> packet), because se_ alloced memory can't be explicitly freed.  If you can't do
> #1 (where the g_ alloced memory is "hidden" from the dissector), I would
> recommend using g_ alloced memory for your fragmentation algorithm.  Even
> better if you can modify the reassembly API to allow your dissector to use it.

This is hard task. I guess we can skip it for now. When API will be completed
then I will update this dissector.

By the way, if there any way to get frame data from another one? For example
copy six bytes from frame 67 on protocol XXX (current frame: 89, protocol YYY).
That may be enough to do not duplicate-copy data anymore.


> 3. Would it be possible to use p_get_proto_data() instead of setting
> pinfo->private_data?

Done. This is exactly what I need. Thanks.


(In reply to comment #8)
> + val_to_str(cr, cr_vals, "unknown"), ...
>   val_to_str(packet_type, packet_type_vals, "unknown packet type"));
>
> val_to_str() takes format string as third argument, for displaying actual
> unknown value. It should be something like: "unknown packet type (%u)",
> if you don't want value it should be: val_to_str_const(..., "unknown packet
> type")
>
> Anyway, you specified all possible values, so no unknown values (for now).

Done: Right, but how to handle that case? I think put "unknown" because it is
good idea (...if somebody remove one/more of values for unknown reason)

>
> You could also use AVCTP_PACKET_TYPE_* defines instead of numeric values for
> packet_type_vals.

Done.

(In reply to comment #9)
> Two quite minor comments:
>
>
> 1. FALSE should be ENC_NA in the following:
>
>
> +    ti = proto_tree_add_item(tree, proto_btavctp, tvb, offset, -1, FALSE);
> +
>

Done.

> 2.  As far as I know, wrapping the call to proto_..._register...() with the
>     'proto... == -1' test is not required.
>
> +   if (proto_btavctp == -1) {
>
>     Or: is this related to something about "dual-use" (regular or plugin) ?

Done: Ok, that was removed, because we do not need that anymore.

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