BeeSeek

Merge lp:~vijit.chauhan/beeseek/fix-603119 into lp:beeseek

  1. fix-603119
  2. Merge into trunk
Proposed by vSC
Status: Merged
Merged at revision: 64
Proposed branch: lp:~vijit.chauhan/beeseek/fix-603119
Merge into: lp:beeseek
Diff against target: 103 lines (+35/-6) (has conflicts)
1 file modified
trackers/sniffer/src/sender.c (+35/-6)
Text conflict in trackers/sniffer/src/sender.c
To merge this branch: bzr merge lp:~vijit.chauhan/beeseek/fix-603119
Reviewer Review Type Date Requested Status
Andrea Corbellini Approve
Review via email: mp+30024@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrea Corbellini (andrea.corbellini) wrote :

Hi and thanks for you contribution, it is really appreciated. The code is more or less OK, however there is one thing that doesn't behave correctly, but probably because there were some misunderstandings.

You check the return value of BfSender_SendRaw() into BfSender_Connect(). Since BfSender_SendRaw is used also elsewhere, I think that it would be better to check the return value of send() into BfSender_SendRaw() itself, and call _Connect() in case of errors.

Feel free to ask questions if you have doubts or questions, I'm here to help. Also, a tip: run bzr whoami ;-)

Thanks again for your efforts!

Revision history for this message
vSC (vijit.chauhan) wrote :

Hi, thanks for your inputs ..

So if I am getting clear, the BfSender_SendRaw() should call back BfSender_Connect(), in case of error.

I think BfSender_Connect() does some other things as well, apart from connecting -> what its name suggests .. It calls BfSender_SendRaw, also does the receiving of data from server ..

It could be restructured to just do just the connecting part .. a parent function could call three different functions for connecting, sending and receiving .. that way it would be easier to manage the code and make future changes easy ..

Well, I hope I have made myself clear :P

Meanwhile, I will try to change the code again as per your comment ..

> Hi and thanks for you contribution, it is really appreciated. The code is more
> or less OK, however there is one thing that doesn't behave correctly, but
> probably because there were some misunderstandings.
>
> You check the return value of BfSender_SendRaw() into BfSender_Connect().
> Since BfSender_SendRaw is used also elsewhere, I think that it would be better
> to check the return value of send() into BfSender_SendRaw() itself, and call
> _Connect() in case of errors.
>
> Feel free to ask questions if you have doubts or questions, I'm here to help.
> Also, a tip: run bzr whoami ;-)

Yes, thanks :)

>
> Thanks again for your efforts!

Revision history for this message
Andrea Corbellini (andrea.corbellini) wrote :

On Thu, Jul 15, 2010 at 7:25 PM, vSC <email address hidden> wrote:
> So if I am getting clear, the BfSender_SendRaw() should call back BfSender_Connect(), in case of error.

Yeah, right. Probably it'd be useful some code to avoid recursive
loops, but this is something we can think about later.

> I think BfSender_Connect() does some other things as well, apart from connecting -> what its name suggests .. It calls BfSender_SendRaw, also does the receiving of data from server ..
>
> It could be restructured to just do just the connecting part .. a parent function could call three different functions for connecting, sending and receiving .. that way it would be easier to manage the code and make future changes easy ..

It's true that BfSender_Connect() doesn't just connect: in fact it
connects, then sends a request to server and finally checks the
response. And this should be done every time. If the connection is
lost, you should both connect and send the request. For this reason, I
don't think that I'd be useful to split BfSender_Connect() into three
functions would be useful: connecting without sending the request
wouldn't produce the expected result.

Do you agree? Feel free to say you don't, of course ;-)

> Well, I hope I have made myself clear :P
>
> Meanwhile, I will try to change the code again as per your comment ..

Sure, everything's clear. Thanks!

--
Ubuntu member  | http://www.ubuntu.com/
BeeSeek member | http://www.beeseek.org/

lp:~vijit.chauhan/beeseek/fix-603119 updated
36. By vSC

2nd try for fix-603119

Revision history for this message
vSC (vijit.chauhan) wrote :

Hi,

I wasn't very comfortable with the recursive approach.

I have done the fix so that can be avoided. :)

Introduced a new function BfSender_MakeConnection() which takes care of the just the connection part and can be called again by BfSender_SendRaw(), in case of any error in connection.

I guess the changes should not break anything, just add a little bit more flexibility.

Reg.
Vijit

Revision history for this message
Andrea Corbellini (andrea.corbellini) wrote :

Thanks, I really appreciate your contribution. However, this approach won't work: _MakeConnection() is not enough to establish a communication with the server. It *must* send the HTTP message, otherwise the server will send a "400 Bad Request" and drop the connection. If want you want to do is avoiding the recursion in _Connect, you could directly use send() instead of _SendRaw().

I know that what I said isn't obvious, because during your tests you can't get the error. So, I've update the testing script to make it more realistic: <http://wiki.beeseek.org/AnalyserTestingScript>. If you use that, you'll see that if the sniffer loses the connection and then reconnects, the script will output an error.

I hope this helps. If I didn't explained the situation well, just let me know. :-)

Thanks again!

lp:~vijit.chauhan/beeseek/fix-603119 updated
37. By vSC

3rd try fix-603119

Revision history for this message
vSC (vijit.chauhan) wrote :

Hi,
I have reverted the changes and done it as per recursive approach. I think I have understood your concern now -- you would like to start the connection again in case the connection breaks down when sniffing is already going on!

Revision history for this message
Andrea Corbellini (andrea.corbellini) wrote :

It looks perfect! Many thanks!

review: Approve
Revision history for this message
vSC (vijit.chauhan) wrote :

Hi,

Thanks again for all your help, it was really awesome ;=)

Do get back to me if you think I can contribute more :-)

Vijit

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'trackers/sniffer/src/sender.c'
2--- trackers/sniffer/src/sender.c 2010-07-07 15:29:14 +0000
3+++ trackers/sniffer/src/sender.c 2010-07-17 16:24:44 +0000
4@@ -13,16 +13,30 @@
5 /* Socket used to send data to the BeeSeek server. */
6 int BfSender_Socket;
7
8+static char server_address_copy[32]="\0";
9+static int connect_port_copy;
10
11 /* Connect to the BeeSeek server. */
12 int
13 BfSender_Connect(char *address, int port)
14 {
15+
16 struct hostent *host;
17 struct sockaddr_in server_addr;
18
19+<<<<<<< TREE
20 #ifdef BfVerbose
21 # ifdef BfDebug
22+=======
23+ /*Copy into global vars to use if
24+ called by sendRaw again*/
25+
26+ strncpy(server_address_copy,address,strlen(address));
27+ server_address_copy[strlen(address)]='\0';
28+ connect_port_copy = port;
29+
30+#ifdef BfDebug
31+>>>>>>> MERGE-SOURCE
32 printf("%s: debug: connecting to server at %s:%d\n", Bf_ProgramName,
33 address, port);
34 # endif
35@@ -30,6 +44,7 @@
36
37 /* Create the socket. */
38 BfSender_Socket = socket(AF_INET, SOCK_STREAM, 0);
39+
40 if (BfSender_Socket < 0) {
41 fprintf(stderr, "%s: error: cannot create socket: %s\n",
42 Bf_ProgramName, strerror(errno));
43@@ -59,8 +74,7 @@
44 }
45
46 /* Initialize the HTTP communication with the server. */
47- if (BfSender_SendRaw(BfSender_MSG_CONNECT) < 0)
48- return -1;
49+ BfSender_SendRaw(BfSender_MSG_CONNECT);
50
51 char data[12];
52 int bytes_recvd;
53@@ -81,6 +95,7 @@
54 }
55 data_len += bytes_recvd;
56 }
57+
58 /* The status code of the response should be 2XX (e.g. "HTTP/1.1 200 OK").
59 */
60 if (data[9] != '2') {
61@@ -88,6 +103,7 @@
62 Bf_ProgramName, data[9], data[10], data[11]);
63 return -1;
64 }
65+
66 return 0;
67 }
68
69@@ -97,13 +113,25 @@
70 int bytes_sent;
71 while (strlen(data) > 0) {
72 bytes_sent = send(BfSender_Socket, data, strlen(data), MSG_NOSIGNAL);
73- if (bytes_sent < 0) {
74+
75+ if (bytes_sent < 0) {
76 fprintf(stderr, "%s: error: cannot send message to server: %s\n",
77 Bf_ProgramName, strerror(errno));
78- return -1;
79- }
80- data += bytes_sent;
81+
82+ BfSender_Close();
83+
84+ /*In case of send error trying connecting again*/
85+ while ( BfSender_Connect(server_address_copy,connect_port_copy) < 0 ) {
86+ /* Sleeping for a little while and then trying reconnect again */
87+ sleep(30);
88+ BfSender_Close();
89+ }
90+ }
91+ else {
92+ data += bytes_sent;
93+ }
94 }
95+
96 return 0;
97 }
98
99@@ -126,3 +154,4 @@
100 # endif
101 #endif
102 }
103+

Subscribers

People subscribed via source and target branches