Re: Return value of a new-style dissector

[Evan Huus <eapache () gmail com>]

On Wed, Jun 25, 2014 at 12:54 PM, Evan Huus <eapache () gmail com> wrote:

> On Wed, Jun 25, 2014 at 12:32 PM, Peter Wu <peter () lekensteyn nl> wrote:
>
> > Hi,
> >
> > Since Pascal's change ("TCP: do desegmentation sanity checks for all sub
> > dissectors types"), the whois dissector was starting to throw:
> >
> > Dissector bug, protocol TCP, in packet 6:
> > epan/dissectors/packet-tcp.c:3953:
> > failed assertion "save_desegment_offset == pinfo->desegment_offset &&
> > save_desegment_len == pinfo->desegment_len"
> >
> > That came from this part:
> >
> >     pinfo->desegment_len = DESEGMENT_UNTIL_FIN;
> >     pinfo->desegment_offset = 0;
> >     return (0);
> >
> > It is likely supposed to mean "well, this packet is mine, please give all
> > data
> > until the connection is closed". The `return 0` is clearly wrong here. But
> > what is the correct value?
> >
> > From epan/packet.h:
> > /*
> >  * Dissector that returns:
> >  *
> >  *      The amount of data in the protocol's PDU, if it was able to
> >  *      dissect all the data;
>
> I think the above line is misleading, it should say "The amount of data in
> the protocol's PDU, if the tvbuff contains a PDU for that protocol". So
> tvb_captured_length is the right return value for whois, because it is the
> amount of data (so far) claimed by the whois dissector.
>
>
> >  *      0, if the tvbuff doesn't contain a PDU for that protocol;
> >  *
> >  *      The negative of the amount of additional data needed, if
> >  *      we need more data (e.g., from subsequent TCP segments) to
> >  *      dissect the entire PDU.
>
> If the tcp dissector respects this (what happens if return is negative
> *and* pinfo->desegment_len is set???) then it should maybe do this instead
> of setting desegment_len at all...

Does this mean we can get rid of the pinfo->desegment stuff once all
dissectors are new-style?


>  */
> > typedef int (*new_dissector_t)(tvbuff_t *, packet_info *, proto_tree *,
> > void *);
> >
> > Almost all callers of call_dissector... ignore its return value, those who
> > care seem only to be interested in a boolean (zero vs non-zero). The
> > dissectors
> > which use DESEGMENT_... do arbitrary things:
> >
> >  - Return 0, this is just a plain error. See whois, finger, snmp,
> > alljoyn, ms-mms.
> >  - Return tvb_length(tvb). See nbns
> >  - Return -pinfo->desegment_len. See ldp.
> >  - Return -1. See sigcomp, rtsp
> >  - Return -2. See fcip.
> >  - Return offset. See ssh, ssl
> >  - Return -DESEGMENT_ONE_MORE_SEGMENT (!!). jxta
> >  - Return the actual bytes that is wanted (offset_next - offset_before).
> > See xot.
> >
> > (Found with `view -p $(grep -rnl DESEGMENT_ONE_MORE_SEGMENT)`)
> >
> > This suggests that something is wrong in the API definition. As it stands
> > now,
> > it really needs a boolean. Can someone clarify this? Is this a bug, an
> > incomplete migration from the old-style dissector or a documentation
> > issue?
> > Did I misunderstood something?
>
> I'm not sure, I didn't write the new API. I think incomplete migration?
>
>
> > Kind regards,
> > Peter
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe