Implement the socket communication #7

polaroi8d · 2016-11-02T22:51:50Z

Implement the socket communication server side in ANSI C. Add a new debugger folder in jerry-core for the debugger files. We need the socket communication between the client and the server to exchange debugger's data.

Implement a simple send function
The backlog argument defines the maximum length to which the queue of pending connections for sock may grow.
jerry_debugger_socket_init() for initialize the communication
jerry_debugger_connection_end() to close the socket

JerryScript-DCO-1.0-Signed-off-by: Levente Orban orbanl@inf.u-szeged.hu

stampho · 2016-11-03T08:11:24Z

jerry-debugger/jerry-client.py

+	print 'Socket created on:', port
+
+	data = client_socket.recv(512)
+	print "RECIEVED:" , data


typo: "received"
moreover, this print won't work with python 3, use print("RECEIVED: %s" % data) instead (if data is a string).

stampho · 2016-11-03T08:13:00Z

jerry-debugger/jerry-client.py

+import socket
+import sys
+
+port = 5001 


there is a trailing whitespace at the line of this line. Use git diff before commit and/or git show before push. Trailing whitespaces are marked with a "red rectangle".

Use capitalized constants. For example, PORT = 5001

stampho · 2016-11-03T08:13:13Z

jerry-debugger/jerry-client.py

+	try:
+		client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+		client_socket.connect(("localhost", port))
+	except socket.error, msg: 


trailing whitespace

stampho · 2016-11-03T08:14:47Z

jerry-debugger/jerry-client.py

+port = 5001 
+
+def main():
+	try:


Do not indent with tab. Use the same indentation as used in other python scripts (eg. tools/build.py uses 4 spaces)

stampho · 2016-11-03T08:15:45Z

jerry-debugger/jerry-client.py

@@ -0,0 +1,22 @@
+import socket


Add #!/usr/bin/env python to the first line

License header is mssing

stampho · 2016-11-03T08:27:04Z

jerry-core/debugger/jerry-debugger.h

+
+extern int remote_init(void);
+extern void connection_closed(void);
+extern void send_to_client(const char* data, uint16_t data_len);


possible style issue: * goes to the right side: const char *data

stampho · 2016-11-03T08:27:43Z

jerry-debugger/jerry-client.py

+def main():
+	try:
+		client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+		client_socket.connect(("localhost", port))


"localhost" should be also stored in a constant. For example: HOST = "localhost"

stampho · 2016-11-03T08:29:06Z

jerry-main/main-unix.c

@@ -23,6 +23,10 @@
 #include "jerry-port.h"
 #include "jerry-port-default.h"

+#ifdef JERRY_DEBUGGER
+ #include "jerry-debugger.h"


possible style issue: do we indent includes inside a guard?

stampho · 2016-11-03T08:30:29Z

jerry-main/main-unix.c

@@ -343,6 +354,11 @@ main (int argc,
  int i;
  int files_counter = 0;

+#ifdef JERRY_DEBUGGER
+  /* Initialize the socket connection */


comment seems to be misindented

stampho · 2016-11-03T08:30:44Z

jerry-main/main-unix.c

@@ -673,6 +689,11 @@ main (int argc,
  jerry_release_value (ret_value);
  jerry_cleanup ();

+#ifdef JERRY_DEBUGGER
+  /* Need to close the socket at the end of the program. */


comment seems to be misindented

zherczeg · 2016-11-07T08:53:22Z

Please add a bit more description about the patch.

zherczeg · 2016-11-07T08:55:28Z

jerry-core/debugger/jerry-debugger.c

+ssize_t byte_send;            /**< size of byte_send */
+size_t data_len;              /**< data lenght */
+bool optval = true;           /**< the arguments optval and optlen are used to access option values for setsockopt() */
+char* data[BUFFER_SIZE];      /**< data buffer */


This is just char not char*. We are not string pointers here, only bytes. The current definition is 128 pointers which points to chars.

zherczeg · 2016-11-07T08:56:43Z

jerry-core/debugger/jerry-debugger.c

+
+  /**
+    * Create an endpoint for communication
+    */


A single line is enough for such comments.

stampho · 2016-11-10T08:37:46Z

jerry-core/debugger/jerry-debugger.c

+#include "jerry-port.h"
+
+#define PORT 5001
+#define BLACKLOG 1


this is still not fixed

stampho · 2016-11-10T08:40:33Z

jerry-core/debugger/jerry-debugger.c

+#define BLACKLOG 1
+#define BUFFER_SIZE 128
+
+int sock;                     /**< return value of socket(), used other methods */


the comment is still not clear. Focus on the function of this variable: why is it used for? what is this good for?
For example, "socket file descriptor for the remote communication"

stampho · 2016-11-10T08:40:59Z

jerry-core/debugger/jerry-debugger.c

+  connected = accept (sock, (struct sockaddr *)&client_addr, &sin_size);
+
+  jerry_port_log (JERRY_LOG_LEVEL_DEBUG,"Connected from: %s:%d\n",
+                  inet_ntoa (client_addr.sin_addr),ntohs (client_addr.sin_port));


style: spaces

stampho · 2016-11-10T08:43:10Z

jerry-core/debugger/jerry-debugger.c

+#define BUFFER_SIZE 128
+
+int sock;                     /**< return value of socket(), used other methods */
+int connected;                /**< return value of the whole socket connection */


This is the file descriptor of the connected socket, see: http://man7.org/linux/man-pages/man2/accept.2.html

Rename the variable and rephrase the comment to make it more descriptive.

stampho · 2016-11-10T08:43:40Z

jerry-core/debugger/jerry-debugger.c

+
+int sock;                     /**< return value of socket(), used other methods */
+int connected;                /**< return value of the whole socket connection */
+ssize_t byte_send;            /**< size of byte_send */


Why is this global?

stampho · 2016-11-10T08:44:49Z

jerry-core/debugger/jerry-debugger.c

+int sock;                     /**< return value of socket(), used other methods */
+int connected;                /**< return value of the whole socket connection */
+ssize_t byte_send;            /**< size of byte_send */
+size_t data_len;              /**< data lenght */


Why is this global?

Moreover, you have a function parameter with the same name (but different type) and it is very misleading.

stampho · 2016-11-10T08:45:47Z

jerry-core/debugger/jerry-debugger.c

+int connected;                /**< return value of the whole socket connection */
+ssize_t byte_send;            /**< size of byte_send */
+size_t data_len;              /**< data lenght */
+bool optval = true;           /**< the arguments optval and optlen are used to access option values for setsockopt() */


seems nobody use this: it is set by the setsockopt but the result is not used. I think it is not needed to be global for now.

stampho · 2016-11-10T08:47:25Z

jerry-core/debugger/jerry-debugger.c

+ssize_t byte_send;            /**< size of byte_send */
+size_t data_len;              /**< data lenght */
+bool optval = true;           /**< the arguments optval and optlen are used to access option values for setsockopt() */
+char data[BUFFER_SIZE];       /**< data buffer */


same as the data_len

zherczeg · 2016-11-14T14:21:14Z

jerry-core/debugger/jerry-debugger.h

+
+extern bool remote_init(void);
+extern void connection_closed(void);
+extern void send_to_client(const char *data, uint16_t data_len);


Space is required before (

zherczeg · 2016-11-14T14:21:59Z

jerry-core/debugger/jerry-debugger.c

+  */
+  if (byte_send != (ssize_t) data_len)
+  {
+    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error, there is some missing package...\n");


I don't understand this error message.

There is a byte_send which has the return value of send ( on success, these calls return the number of bytes sent ). I check the data_len with this, and if the two value is not equal there is some missing bytes. Maybe I need to add to check the -1 ( error ), what do you think?

zherczeg · 2016-11-14T14:22:35Z

jerry-core/debugger/jerry-debugger.c

+
+
+/* Send the parsed file names to the client side */
+void send_to_client (const char *data, uint16_t data_len)


Only data_len is needed since we have only one sender buffer.

I don't get it.

zherczeg · 2016-11-14T14:23:35Z

jerry-core/debugger/jerry-debugger.c

+
+  /* Server adress declaration */
+  server_addr.sin_family = AF_INET;             /**< the address family is host by order*/
+  server_addr.sin_port = htons (PORT);           /**< host to network long (PORT) */


Comment misaligned.

zherczeg · 2016-11-17T13:13:38Z

jerry-core/debugger/jerry-debugger.c

+  server_addr.sin_family = AF_INET;
+  server_addr.sin_port = htons (PORT);
+  server_addr.sin_addr.s_addr = INADDR_ANY;
+  bzero (&(server_addr.sin_zero), BACKLOG);


What is this exactly doing?

zherczeg · 2016-11-17T13:14:23Z

jerry-core/debugger/jerry-debugger.c

+int connected;                /**< hold the file descriptor for the accepted socket */
+
+struct sockaddr_in server_addr, client_addr;  /**< declarations of the socket address */
+socklen_t sin_size = sizeof (struct sockaddr_in);  /**< size of the structure pointed by


Why do we need to create a non-static non-const variable for this? I think we can simply use sizeof (struct sockaddr_in) when needed.

zherczeg · 2016-11-17T13:15:18Z

jerry-core/debugger/jerry-debugger.c

+#define BACKLOG 1
+
+int sock;                     /**< socket file descriptor for the remote communication */
+int connected;                /**< hold the file descriptor for the accepted socket */


These should be static. Perhaps they could even be part of the global context but I am not sure.

zherczeg · 2016-11-17T13:16:13Z

jerry-core/debugger/jerry-debugger.c

+int sock;                     /**< socket file descriptor for the remote communication */
+int connected;                /**< hold the file descriptor for the accepted socket */
+
+struct sockaddr_in server_addr, client_addr;  /**< declarations of the socket address */


I think these could be local variables of main().

zherczeg · 2016-11-17T13:16:33Z

jerry-core/debugger/jerry-debugger.c

+  /* Set the options on socket */
+  if (setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof (int)) == -1)
+  {
+    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Erorr: %s\n", strerror (errno));


zherczeg · 2016-11-17T13:18:02Z

jerry-core/debugger/jerry-debugger.c

+{
+  jerry_port_log (JERRY_LOG_LEVEL_DEBUG, "TCPServer connection closed on port: %d\n", PORT);
+  close (connected);
+  close (sock);


Since we support only one connection I think there is no point to keep the listening socket.
Also connected should be renamed something else, such es jerry_debugger_socket

zherczeg · 2016-11-17T13:20:04Z

jerry-core/debugger/jerry-debugger.c

+   * information is sent to the client. */
+  else if (byte_send != (ssize_t) data_len)
+  {
+    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: Missing bytes from the pacakge.\n");


package.
Ot just say: Error: transmission error.

Btw, I think a loop would be better here to send the remaining bytes. Only -1 is a true error, partial transmission could be resumed.

zherczeg · 2016-11-17T13:20:45Z

jerry-core/debugger/jerry-debugger.h

+
+extern bool remote_init (void);
+extern void connection_closed (void);
+extern void send_to_client (uint16_t data_len);


this can be size_t instead of uint16_t. Both are one register long.

zherczeg · 2016-11-18T09:26:01Z

jerry-core/debugger/jerry-debugger.h

@@ -21,6 +21,12 @@

 #define MAX_BUFFER_SIZE 128

+static uint8_t jerry_debugger_buffer[MAX_BUFFER_SIZE];


To avoid duplicate definitions this should only be an extern definition:

extern uint8_t jerry_debugger_buffer[MAX_BUFFER_SIZE];

In the C file you should create the variable:

uint8_t jerry_debugger_buffer[MAX_BUFFER_SIZE];

Also comment about the variable is mission.

JerryScript-DCO-1.0-Signed-off-by: Levente Orban orbanl@inf.u-szeged.hu

stampho suggested changes Nov 3, 2016

View reviewed changes

zherczeg reviewed Nov 7, 2016

View reviewed changes

jerry-core/debugger/jerry-debugger.c

/**

* Create an endpoint for communication

*/

Copy link

zherczeg Nov 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single line is enough for such comments.

polaroi8d force-pushed the socket-client branch 2 times, most recently from 1ccd9b4 to 02189c0 Compare November 9, 2016 07:50

stampho suggested changes Nov 10, 2016

View reviewed changes

zherczeg reviewed Nov 14, 2016

View reviewed changes

polaroi8d force-pushed the socket-client branch 3 times, most recently from b4fddb4 to f6df650 Compare November 17, 2016 12:47

zherczeg reviewed Nov 17, 2016

View reviewed changes

polaroi8d force-pushed the socket-client branch 2 times, most recently from c911b08 to 1e5938f Compare November 18, 2016 07:08

polaroi8d closed this Nov 18, 2016

polaroi8d force-pushed the socket-client branch from 1e5938f to 7b15eb2 Compare November 18, 2016 07:22

polaroi8d reopened this Nov 18, 2016

zherczeg reviewed Nov 18, 2016

View reviewed changes

Implement the socket communication

a21433f

JerryScript-DCO-1.0-Signed-off-by: Levente Orban orbanl@inf.u-szeged.hu

polaroi8d force-pushed the socket-client branch from c27588b to a21433f Compare November 18, 2016 10:51

polaroi8d merged commit 7a36672 into remote-debugger Nov 23, 2016

polaroi8d deleted the socket-client branch January 6, 2017 08:49

polaroi8d pushed a commit that referenced this pull request Jan 18, 2017

Implement the socket communication (#7)

1be3371

JerryScript-DCO-1.0-Signed-off-by: Levente Orban orbanl@inf.u-szeged.hu



		/* Send the parsed file names to the client side */
		void send_to_client (const char *data, uint16_t data_len)

		@@ -21,6 +21,12 @@

		#define MAX_BUFFER_SIZE 128

		static uint8_t jerry_debugger_buffer[MAX_BUFFER_SIZE];

Implement the socket communication #7

Implement the socket communication #7

Uh oh!

Conversation

polaroi8d commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

polaroi8d commented Nov 2, 2016 •

edited

Loading