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
