Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 6156] Dissector for HDFS

[bugzilla-daemon@xxxxxxxxxxxxx]

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

--- Comment #8 from Bill Meier <wmeier@xxxxxxxxxxx> 2011-08-04 15:14:04 EDT ---
Addt'l comments:

1. In general, tvb_reported_length() should be used instead of
   tvb_length().
   tvb_length() reports the actual amount data captured (which may
   be less than the real size of the frame if s 'snapshot length'
   was used during the capture.
   tvb_reported_length() gives the real length of the frame (even if
   only the first part was captured/saved).

   That being said:
     In general, a much better style iso constantly checking 
     tvb_reported_length() is to just proceed with the dissection
     based upon the length fields in the frame, adjusting 'offset'
     as you go. If something goes wrong (e.g., less bytes available
     in a field than the length field specifies), the dissector will throw
     an exception when trying to access the field and a "malformed"
     indication will be shown for the frame.
     For TCP (a "streaming" protocol), depending upon the
     frame length to know how to proceed is normally
     (almost always ?) not the right idea.
     (A quick look suggests that your code depends
      upon all the data for a message being present in one frame. 
      This might be the case most of the time, but is not guaranteed.
      However, let's leave that for later).

2. Compile warnings on Windows using VC2008

packet-hdfs.c(102) : warning C4018: '<=' : signed/unsigned mismatch
packet-hdfs.c(119) : warning C4018: '<=' : signed/unsigned mismatch
packet-hdfs.c(133) : warning C4018: '<=' : signed/unsigned mismatch

  (These probably get addressed after reworking the code as described
   in item #1 above).

3.Please strip trailing whitespace from lines.

4. It appears that TCP port 8020 is assigned by IANA as follows:

  intu-ec-svcdisc 8020/tcp    Intuit Entitlement Service and Discovery
  intu-ec-svcdisc 8020/udp    Intuit Entitlement Service and Discovery
  intu-ec-client  8021/tcp    Intuit Entitlement Client
  intu-ec-client  8021/udp    Intuit Entitlement Client

  http://www.iana.org/assignments/port-numbers

   So: I suggest adding a preference to specify the TCP port to be used.
       If there's no assigned port (which I have the impression is the case)
       then the dissector_add_uint() should be called only if the 
       TCP port preference is set to non-zero.

    See epan/dissectors/packet-fcgi.c for an example.

5. IMHO 'if (!success)' to test for "status == success" is kind of
   misleading (but perfectly valid C) to someone casually reading the code.
   Maybe: 'if (success == 0)' or even
          'if (status == 0)'  ??

6. Please use tvb_memeql() iso tvb_get_ptr()/strncmp() & etc

Let's start with the above and then we'll see if there are further comments.


Writing Wireshark dissectors takes a bit of work to learn the style and
the API but we'll get there....    :)

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