On 01/16/2013 08:42 PM, Scott Talbert wrote:
> On Sat, 12 Jan 2013, Phil Dibowitz wrote:
> 
>>> Also, looking in my 'pending patches' directory, there are still a couple
>>> there that haven't been merged: the Harmony 300 patch (v4) and the patch
>>> to do config dumping for zwave (v6).  Although, these probably aren't
>>> super critical to get into a release, although the 300 support would be
>>> nice.
>>
>> Oh... I'll look for those, sorry.
> 
> Here's the config dumping patch.

+       /*
+        * Subtract both TCP (4) and UDP (2) headers from pkt[0] which is
+        *  len - 1.  (len = pkt[0] + 1 - 6)
+        */
+       len = pkt[0] - 5;

That comment uses 'len' to mean two different things... which is SUPER
confusing. I had to read that several times to parse it correctly.

Also, you include the "size" header in your calculation of 'len', but I think
you mean "payload_size" which, if I understand correctly, would be pkt[0] - 6

-       memcpy(data, pkt + 1, len+3);
+       memcpy(data, pkt + 1, len+5); // include headers, minus the size

So wait - pkt[0] holds "length - 1", or length not including itself, or index
of the last byte, however you want to look at it.

So the size is is len+7 (6 bytes of header, plus the byte that holds the
size-1), right? so if the payload is say, 8, the tcp/udp headers are 6, the
first byte is "14" and the whole packet is actually 15 bytes.

So if you start at pkt+1, and you want the headers you want len - 1 + 6 bytes,
which is indeed len+5. It's confusing that this number changed by 2 when you
changed 'len' by 1, but I guess the old code was just broken.

That said, I THINK it's more obvious what's going on if the code goes like this:

  /*
   * pkt[0] describes the length of the _rest_ of the packet, not including
   * itself.
   * To get the payload size we subtract both the TCP (4) and UDP (2) headers:
   */
  payload_size = pkt[0] - 6;
  last_seq = pkt[2];
  last_ack = pkt[3];
  last_payload_bytes = payload_size + 3; // tcp payload size
  memcpy(data, pkt + 1, payload_size + 6); // plus headers
  return 0;

Or better yet, lets make two #defines:

  #define USBNET_TCP_HDR_SIZE 4
  #define USBNET_UCP_HDR_SIZE 2

And then the code becomes:

  /*
   * pkt[0] describes the length of the _rest_ of the packet, not including
   * itself.
   * To get the payload size we subtract both the TCP and UDP headers:
   */
  payload_size = pkt[0] - USBNET_TCP_HDR_SIZE - USBNET_UCP_HDR_SIZE;
  last_seq = pkt[2];
  last_ack = pkt[3];
  last_payload_bytes = payload_size + 3; // tcp payload size
  memcpy(data, pkt + 1,
         payload_size + USBNET_TCP_HDR_SIZE - USBNET_UCP_HDR_SIZE);
  return 0;


+// Searches for the "end of file" sequence in a set of two packets.  Returns
+// the number of bytes in the second packet up to and including the sequence.
+// Returns 0 if not found.  Parameters point to the data section of the 
packets.
+// Note for the first packet, only the last 3 bytes are provided.
+int FindEndSeq(uint8_t *pkt_1, uint8_t *pkt_2)
+{
+       uint8_t end_seq[4] = { 0x44, 0x4B, 0x44, 0x4B }; // end of file sequence
+       uint8_t tmp[57]; // 3 bytes from the 1st packet, 54 bytes from the 2nd
+       memcpy(&tmp, pkt_1, 3);
+       memcpy(&tmp[3], pkt_2, 54);
+       for (int i=0; i<54; i++) {
+               if (memcmp(&end_seq, &tmp[i], 4) == 0) {
+                       return i + 1;
+               }
+       }
+       return 0;
+}
+

This ... is strange to me. For starters, you look at the FIRST 3 bytes of the
first packet, not the last 3 as stated in the comment. Second of all... Why do
you look at 3 bytes plus a full packet?

+int CRemoteZ_HID::ReadRegion(uint8_t region, uint32_t &rgn_len, uint8_t *rd,
+       lc_callback cb, void *cb_arg, uint32_t cb_stage)

You're still defining this as virtual:

+       virtual int ReadRegion(uint8_t region, uint32_t &len, uint8_t *rd,
+               lc_callback cb, void *cb_arg, uint32_t cb_stage);



+               if (!eof_found) {
+                       eof_found = FindEndSeq(prev_pkt_tail, &rsp[5]);
+                       if (eof_found) {
+                               rlen = eof_found;
+                       }
+                       rgn_len += rlen;
+                       memcpy(&prev_pkt_tail, &rsp[56], 3);
+
+                       if (rd) {
+                               memcpy(rd_ptr, &rsp[5], rlen);
+                               rd_ptr += rlen;
+                       }
+               }

Coming back to the FindEndSeq code from above... can you comment here whats'
going on? Or more importantly, why? :)


+       /* Return TCP state to initial conditions */
+       SYN_ACKED = false;

I know you said this isn't necessary to clear elsewhere, but while you're here
you want to clean it up in UpdateConfig?

-- 
Phil Dibowitz                             p...@ipom.com
Open Source software and tech docs        Insanity Palace of Metallica
http://www.phildev.net/                   http://www.ipom.com/

"Be who you are and say what you feel, because those who mind don't matter
 and those who matter don't mind."
 - Dr. Seuss


Attachment: signature.asc
Description: OpenPGP digital signature

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel

Reply via email to