On Tue, 5 Feb 2013, Phil Dibowitz wrote:
+ /*
+ * 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.
I agree the comment is confusing now that I read it again. :-) Addressed
below.
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.
Your understanding is close, but not quite right. The size byte is
included in the TCP header byte count, so payload size is really pkt[0] -
TCP_HDR_SIZE - UDP_HDR_SIZE + 1. Yes, the old code was just broken. I
don't think 'len' was actually ever used by the old code, so it didn't
hurt anything.
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;
I ended up revising this section a bit - not quite what you did above, but
hopefully it made things clearer.
+// 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?
No, I don't. See where FindEndSeq is called - the last 3 bytes of the
first packet are passed in. As to your second question, I'll explain
below.
+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);
I mainly did that for consistency with the rest of the class declarations
in remote.h (take a look there). I guess it doesn't really need to be
virtual unless we plan to subclass CRemoteZ_HID someday, but it probably
doesn't hurt anything. No?
+ 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? :)
Okay, here's what's going on. In order to determine where to stop reading
the config, I need to search for a sequence of four bytes. Otherwise, the
remote will keep sending me data, but it is all zeroes, or garbage or
something. FindEndSeq() basically searches packets for this sequence of
four bytes. The tricky part is that the sequence could be spread across 2
packets, so I could miss it if I am just looking at each packet
individually. Thus, in any given round, I look at the last three bytes
from the previous packet (because I would have found it in the previous
round if it was in the last four bytes or earlier) plus the current
packet. Thus, if it crosses packet boundaries or is in the current packet
completely, I'll find it. I'll update the comments to explain this a
little better.
+ /* 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?
Done.
v7 attached.
Implemented config dumping for zwave-hid remotes. Implemented the ReadRegion
function for zwave-hid, based on the usbnet version and the zwave-hid
UpdateConfig function.
Signed-off-by: Scott Talbert <s...@techie.net>
Index: libconcord/libconcord.cpp
===================================================================
RCS file: /cvsroot/concordance/concordance/libconcord/libconcord.cpp,v
retrieving revision 1.42.2.24
diff -u -p -r1.42.2.24 libconcord.cpp
--- libconcord/libconcord.cpp 12 Jan 2013 23:54:55 -0000 1.42.2.24
+++ libconcord/libconcord.cpp 8 Feb 2013 05:09:21 -0000
@@ -564,7 +564,7 @@ int _fix_magic_bytes(uint8_t *in, uint32
*/
int is_config_dump_supported()
{
- return (is_z_remote() && !is_usbnet_remote()) ? LC_ERROR_UNSUPP: 0;
+ return 0;
}
int is_config_update_supported()
@@ -945,6 +945,16 @@ int read_config_from_remote(uint8_t **ou
cb_arg = (void *)true;
}
+ // For zwave-hid remotes, need to read the config once to get the size
+ // For usbnet we do this in GetIdentity, but for hid it takes too long
+ if (is_z_remote() && !is_usbnet_remote()) {
+ if (err = ((CRemoteZ_HID*)rmt)->ReadRegion(REGION_USER_CONFIG,
+ ri.config_bytes_used, NULL, cb,
+ cb_arg, LC_CB_STAGE_READ_CONFIG)) {
+ return err;
+ }
+ }
+
*size = ri.config_bytes_used;
*out = new uint8_t[*size];
Index: libconcord/remote.h
===================================================================
RCS file: /cvsroot/concordance/concordance/libconcord/remote.h,v
retrieving revision 1.20.2.11
diff -u -p -r1.20.2.11 remote.h
--- libconcord/remote.h 4 Jun 2012 07:09:49 -0000 1.20.2.11
+++ libconcord/remote.h 8 Feb 2013 05:09:21 -0000
@@ -31,6 +31,8 @@
#define FIRMWARE_MAX_SIZE 64*1024
/* Largest packet size for HID-UDP is 4 bytes (header) + 64 bytes (data) */
#define HID_UDP_MAX_PACKET_SIZE 68
+#define HID_UDP_HDR_SIZE 2
+#define HID_TCP_HDR_SIZE 4
/* Largest packet size for usbnet is the COMMAND_WRITE_UPDATE_DATA
which is 1 (num params) + 3 (3 parameter size bytes) + 1 (param 1)
+ 1024 (param 2) + 4 (param 3) = 1033. */
@@ -326,8 +328,6 @@ protected:
virtual int ParseParams(uint32_t len, uint8_t *data,
TParamList &pl);
virtual uint16_t GetWord(uint8_t *x) { return x[1]<<8 | x[0]; };
- virtual int ReadRegion(uint8_t region, uint32_t &len, uint8_t *rd,
- lc_callback cb, void *cb_arg, uint32_t cb_stage) {return 0;};
public:
CRemoteZ_HID() {};
@@ -335,6 +335,8 @@ public:
int UpdateConfig(const uint32_t len, const uint8_t *wr,
lc_callback cb, void *cb_arg, uint32_t cb_stage);
int IsUSBNet() {return false;}
+ virtual int ReadRegion(uint8_t region, uint32_t &len, uint8_t *rd,
+ lc_callback cb, void *cb_arg, uint32_t cb_stage);
};
// 1000, 1000i
@@ -358,9 +360,6 @@ public:
virtual ~CRemoteZ_USBNET() {};
int UpdateConfig(const uint32_t len, const uint8_t *wr,
lc_callback cb, void *cb_arg, uint32_t cb_stage);
- int ReadFlash(uint32_t addr, const uint32_t len, uint8_t *rd,
- unsigned int protocol, bool verify=false,
- lc_callback cb=NULL, void *cb_arg=NULL, uint32_t cb_stage=0);
int GetTime(const TRemoteInfo &ri, THarmonyTime &ht);
int SetTime(const TRemoteInfo &ri, const THarmonyTime &ht,
lc_callback cb=NULL, void *cb_arg=NULL, uint32_t cb_stage=0);
Index: libconcord/remote_info.h
===================================================================
RCS file: /cvsroot/concordance/concordance/libconcord/remote_info.h,v
retrieving revision 1.12.2.4
diff -u -p -r1.12.2.4 remote_info.h
--- libconcord/remote_info.h 4 Jun 2012 07:09:49 -0000 1.12.2.4
+++ libconcord/remote_info.h 8 Feb 2013 05:09:21 -0000
@@ -348,7 +348,7 @@ static const TArchInfo ArchList[]={
0x000110, // serial_address
0x000000, // flash_base
0x010000, // firmware_base
- 0x020000, // config_base
+ REGION_USER_CONFIG, // config_base
0, // firmware_update_base
0, // firmware_4847_offset
0x1, /* hack */ // cookie
Index: libconcord/remote_z.cpp
===================================================================
RCS file: /cvsroot/concordance/concordance/libconcord/remote_z.cpp,v
retrieving revision 1.25.2.17
diff -u -p -r1.25.2.17 remote_z.cpp
--- libconcord/remote_z.cpp 12 Jan 2013 23:50:33 -0000 1.25.2.17
+++ libconcord/remote_z.cpp 8 Feb 2013 05:09:21 -0000
@@ -207,13 +207,20 @@ int CRemoteZ_HID::TCP_Read(uint8_t &stat
if (pkt[0] < 3) {
return LC_ERROR;
}
- len = pkt[0] - 4;
+ /*
+ * pkt[0] is the index of the last byte, which means it is equal to the
+ * length of packet minus one byte. 'len' is expected to be set to the
+ * payload size. To get the payload size we subtract both the TCP and
+ * UDP headers from pkt[0] and then add one.
+ */
+ len = pkt[0] - HID_TCP_HDR_SIZE - HID_UDP_HDR_SIZE + 1;
last_seq = pkt[2];
last_ack = pkt[3];
- last_payload_bytes = len + 1; // tcp payload size
+ last_payload_bytes = len + HID_UDP_HDR_SIZE; // tcp payload size
//if(!len) return 0;
//memcpy(data, pkt + 6, len);
- memcpy(data, pkt + 1, len+3);
+ // include headers, minus the size
+ memcpy(data, pkt + 1, len + HID_TCP_HDR_SIZE + HID_UDP_HDR_SIZE - 1);
return 0;
}
@@ -577,14 +584,6 @@ int CRemoteZ_USBNET::SetTime(const TRemo
return 0;
}
-int CRemoteZ_USBNET::ReadFlash(uint32_t addr, const uint32_t len, uint8_t *rd,
- unsigned int protocol, bool verify, lc_callback cb,
- void *cb_arg, uint32_t cb_stage)
-{
- uint32_t tmp;
- return ReadRegion(addr, tmp, rd, cb, cb_arg, cb_stage);
-}
-
int CRemoteZ_USBNET::ReadRegion(uint8_t region, uint32_t &rgn_len, uint8_t *rd,
lc_callback cb, void *cb_arg, uint32_t cb_stage)
{
@@ -910,6 +909,170 @@ int CRemoteZ_Base::ReadFlash(uint32_t ad
unsigned int protocol, bool verify, lc_callback cb,
void *cb_arg, uint32_t cb_stage)
{
+ uint32_t tmp;
+ return ReadRegion(addr, tmp, rd, cb, cb_arg, cb_stage);
+}
+
+/*
+ * When reading a config from the remote, we need to look for a sequence of
+ * four bytes to determine when to stop reading the config. The tricky part is
+ * that the sequence could be split across two packets. Thus, we need to
+ * search the last three bytes of the previous packet plus the current packet.
+ * This function searches for the "end of file" sequence in a set of two
+ * packets. If the sequence is found, returns the number of bytes in the
+ * second packet up to and including the sequence. Returns 0 if the sequence
+ * is not found. Parameters point to the data section of the packets. Note
for
+ * the first packet, only the last 3 bytes are expected to be passed in.
+ */
+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;
+}
+
+int CRemoteZ_HID::ReadRegion(uint8_t region, uint32_t &rgn_len, uint8_t *rd,
+ lc_callback cb, void *cb_arg, uint32_t cb_stage)
+{
+ int err = 0;
+ int cb_count = 0;
+ uint8_t rsp[60];
+ unsigned int rlen;
+ uint8_t status;
+ CRemoteZ_Base::TParamList pl;
+
+ /* Start a TCP transfer */
+ if ((err = Write(TYPE_REQUEST, COMMAND_INITIATE_UPDATE_TCP_CHANNEL))) {
+ debug("Failed to write to remote");
+ return LC_ERROR_WRITE;
+ }
+
+ /* Make sure the remote is ready to start the TCP transfer */
+ if ((err = Read(status, rlen, rsp))) {
+ debug("Failed to read from remote");
+ return LC_ERROR_READ;
+ }
+
+ if (rsp[1] != TYPE_RESPONSE || rsp[2] !=
+ COMMAND_INITIATE_UPDATE_TCP_CHANNEL) {
+ return LC_ERROR;
+ }
+
+ /* Look for a SYN packet */
+ debug("Looking for syn");
+ if ((err = TCP_Read(status, rlen, rsp))) {
+ debug("Failed to read syn from remote");
+ return LC_ERROR_READ;
+ }
+
+ if (rsp[0] != TYPE_TCP_SYN) {
+ debug("Not a SYN packet!");
+ return LC_ERROR;
+ }
+
+ /* ACK it with a command to read a region */
+ debug("READ_REGION");
+ // 1 parameters, 1 byte, region to read.
+ uint8_t cmd[60] = { region };
+ if ((err = TCP_Write(TYPE_REQUEST, COMMAND_READ_REGION, 1, cmd))) {
+ debug("Failed to write to remote");
+ return LC_ERROR_WRITE;
+ }
+ if ((err = TCP_Read(status, rlen, rsp))) {
+ debug("Failed to read to remote");
+ return LC_ERROR_READ;
+ }
+ if (rsp[0] != TYPE_TCP_ACK || rsp[3] != TYPE_RESPONSE ||
+ rsp[4] != COMMAND_READ_REGION) {
+ debug("Incorrect response type from remote");
+ return LC_ERROR_INVALID_DATA_FROM_REMOTE;
+ }
+
+ rgn_len = 0;
+
+ debug("READ_REGION_DATA");
+ int data_read = 0;
+ uint8_t *rd_ptr = rd;
+ cmd[0] = region;
+ int eof_found = 0;
+ uint8_t prev_pkt_tail[3] = { 0x00, 0x00, 0x00 };
+
+ while (1) {
+ if ((err = TCP_Write(TYPE_REQUEST, COMMAND_READ_REGION_DATA, 1,
+ cmd))) {
+ debug("Failed to write to remote");
+ return LC_ERROR_WRITE;
+ }
+ if ((err = TCP_Read(status, rlen, rsp))) {
+ debug("Failed to read to remote");
+ return LC_ERROR_READ;
+ }
+ if (rsp[0] != TYPE_TCP_ACK || rsp[3] != TYPE_RESPONSE ||
+ ((rsp[4] != COMMAND_READ_REGION_DATA) &&
+ (rsp[4] != COMMAND_READ_REGION_DONE))) {
+ debug("Incorrect response type from remote");
+ return LC_ERROR_INVALID_DATA_FROM_REMOTE;
+ }
+ if (rsp[4] == COMMAND_READ_REGION_DONE) {
+ break;
+ }
+ data_read += rlen;
+
+ 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;
+ }
+ }
+
+ debug("DATA %d, read %d bytes, %d bytes total", cb_count,
+ rlen, data_read);
+
+ if (cb) {
+ cb(cb_stage, cb_count++, data_read, data_read+1,
+ LC_CB_COUNTER_TYPE_BYTES, cb_arg, NULL);
+ }
+ }
+
+ debug("FIN-ACK");
+ if ((err = TCP_Ack(false, true))) {
+ debug("Failed to send fin-ack");
+ return LC_ERROR_WRITE;
+ }
+
+ if ((err = TCP_Read(status, rlen, rsp))) {
+ debug("Failed to read from remote");
+ return LC_ERROR_READ;
+ }
+
+ /* Make sure we got an ack */
+ if (rsp[0] != (TYPE_TCP_ACK | TYPE_TCP_FIN)) {
+ debug("Failed to read finish-update ack");
+ return LC_ERROR;
+ }
+
+ if ((err = TCP_Ack(true, false))) {
+ debug("Failed to ack the ack of our fin-ack");
+ return LC_ERROR_WRITE;
+ }
+
+ /* Return TCP state to initial conditions */
+ SYN_ACKED = false;
+
return 0;
}
@@ -1320,6 +1483,9 @@ int CRemoteZ_HID::UpdateConfig(const uin
* it never comes... so we don't bother trying to read it.
*/
+ /* Return TCP state to initial conditions */
+ SYN_ACKED = false;
+
return 0;
}
------------------------------------------------------------------------------
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
