[bugzilla-daemon@xxxxxxxxxxxxx]

Bill Meier changed bug 8240

What	Removed	Added
Status	IN_PROGRESS	INCOMPLETE

Comment # 7 on bug 8240 from Bill Meier

Ok. Looks like a solid base, altho some work needed....

Some comments (in no particular order);

1. global protocol_tcp is referenced in dissect_openvpn() before being set in
   dissect_openvpn_msg(0.

   It would seem to me that dissect_openvpn() needs to use the same test
   as in dissect_openvpn_msg(). Also, if so, then protocol_tcp need not be
   a global.

2. __attribute__ is for gcc. For portability (e.g., to compile on Windows)
   use _U_ instead. _U_ is defined appropriately for different 
   platforms.

3. ssl_handle = find_dissector("ssl"); should be done only
   once with ssl_handle being a static global.

4. It would be appreciated if you would strip trailing whitespace.

5. Please provide a reference to the protocol specification (in a comment
   in the source).

6. Copyright:

   - You should add a date (year).
   - I'm guessing that the copyright is by the 5 individuals named and does
     not include the University. Is this correct ?
     If any case, the " *    Semester Project ... " text should probably be
more
     clearly separated from the (c) paragraph.

7. issues detected by fix-encoding-args:

   proto_tree_add_item(data_tree, hf_openvpn_data, next_tvb, offset, -1, \
        [[ENC_BIG_ENDIAN]-->[ENC_NA]]);

   proto_tree_add_item(openvpn_tree, hf_openvpn_hmac, tvb, offset, \
        tls_auth_hmac_size, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);

   tvb_get_bits32(tvb, offset*8+32, 32, [[FALSE]-->[ENC_BIG_ENDIAN]]);

   tvb_get_bits32(tvb, offset*8, 32, [[FALSE]-->[ENC_BIG_ENDIAN]]);

8. @ ~ line 305 tvb_length_remaining() is called 3x.
   It would be a bit nicer to call it once to call it just once).

9. The following statement bothered me a bit:

    msg_lastframe = (tvb_length_remaining(tvb, offset) == 100 ? FALSE : TRUE);

    I believe tt works because the tvb (either UDP or TCP) will have
    exactly one openvpn PDU; (UDP: by definition; TCP: because of the use 
    of tcp_dissect_pdus()).

    I suggest adding a comment so that a casual reader of the code will
    understand why the test works.

10. Only prefs vars should be under the /* Preferences */ comment.
    The other stuff, ett vars, etc should be moved separately as appropiate.

11. proto_tree_add_item(data_tree, hf_openvpn_data, next_tvb, offset, \
         tvb_length_remaining(next_tvb, offset), ENC_BIG_ENDIAN);

    -1 can be used instead of tvb_length_remaining()

12.  fragment_msgid use looks problematical:
    - never initialized to zero (except after a PDU is re-assembled and
      for the initial use of the dissector).
      (This presumably might be handled by initializing the variable to 0 in
      the init routine)
. 
    - *However*: storing fragment_msgid as a global would seem to me to
               be a non-starter.

      Reason: I presume multiple UDP (?sessions) each with a different
              msg_sessionid value can occur simultaneously.

              If so, you'll need to keep the fragment_msgid separately for
              each sessionid (maybe per sessionid using a hash table or
              maybe per UDP "conversation" if the UDP src/dest addr/port
              quadruple will always be unique for a session).



14. tvb_new_subset(tvb, offset, -1, -1)  --> tvb_new_subset_remaining(...)

15. Relevant comments in Bug %8239 (wiki update, modelines, AUTHORS...) also
   apply to this dissector.


That's it for now. If you have any questions please let us now.

---

      You are receiving this mail because:

• You are watching all bug changes.