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
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