Ci lamp dev clean #13

SParmentier · 2016-10-20T09:28:30Z

@phalox Could you review this ?

…process uri

…odules into ci_lamp_dev_clean

phalox

Unit tests I didn't properly review. Could you reply to my comments and fix where applicable?

phalox · 2016-10-21T09:20:49Z

libhttp/Makefile

@@ -9,7 +9,7 @@ TEST_LDFLAGS=-pthread $(PREFIX)/modules/*.o $(PREFIX)/lib/*.o -lvdeplug -lpcap
 OPTIONS?=
 UNITS_DIR=../build/test/units

-CFLAGS=-I$(PICOTCP)/build/include
+CFLAGS=-I$(PICOTCP)/build/include -I../../../out/include -I../../wolfssl


I don't like the ../../.. a lot. Do we refer to the output directory while building or should we be using the .h files from the source code?
@frederikvs @mkrrr your take?

going to something similar to the $(PICOTCP) would already be an improvement IMO. Create an environment var to hold the path to other libraries, use that in the -I.

phalox · 2016-10-21T09:22:19Z

libhttp/pico_http_client.c

+/* Uncomment the following include to compile picotcp-modules in
+ * ci_lamp_dev_clean branch on its own and not with tass-connected-device
+ * */
+//#include <wolfcrypt/src/coding.c>


@VincentDeHaen Any clue how we can do this less hackerish? The code should work nomatter where we are compiling it from...

phalox · 2016-10-21T09:23:18Z

libhttp/pico_http_client.c

@@ -277,6 +283,11 @@ static int8_t pico_http_uri_destroy(struct pico_http_uri *urikey)
 PICO_FREE(urikey->host);
 urikey->host = NULL;
 }
+ if (urikey->user_pass)
+ {
+ PICO_FREE(urikey->resource);


This is a bug. Please fix the ->resource

phalox · 2016-10-21T09:24:14Z

libhttp/pico_http_client.c

@@ -330,7 +341,9 @@ static int8_t pico_process_uri(const char *uri, struct pico_http_uri *urikey)
 dbg("Start: pico_process_uri(..) %s\n", uri);
 if (!uri || !urikey || uri[0] == '/')
 {
+ if (urikey) pico_http_uri_destroy(urikey);


Not sure what the code style policy is, but I prefer the explicit use of { } and to put each thing on its own line

Also, it's a bit confusing to first check on the possible non-existance of the urikey variable, and then to check on that it exists. Can you write the code differently? (could be that I'm nitpicking, then just ignore this comment :-))

Use of { } is debatable, I don't really mind them missing, but it's definitely more readable if the body of the if is on its own line.

phalox · 2016-10-21T09:27:45Z

libhttp/pico_http_client.c

@@ -378,27 +392,22 @@ static int8_t pico_process_uri(const char *uri, struct pico_http_uri *urikey)
 index++;


Phew this function is getting very long. Could you rework this into different smaller functions? (or at least create a jira ticket for it)

phalox · 2016-10-21T09:43:51Z

libhttp/pico_http_client.c

- {
- client->connectionID = global_client_conn_ID++;
- }
+ client->connectionID = connID >= 0 ? connID : global_client_conn_ID++;


Here brackets would improve readability. Just to verify: you know that global_client_conn_ID is only incremeted AFTER it's assigned? That's ok?

phalox · 2016-10-21T09:44:17Z

libhttp/pico_http_client.c

@@ -1895,15 +1909,15 @@ static inline int32_t read_chunked_data(struct pico_http_client *client, unsigne
 {
 uint32_t len_read = 0;
 int32_t value;
- /* read the chunk line */
+ // read the chunk line


I think these comments were actually done on purpose. The old comments are the actual proper C-style comments

phalox · 2016-10-21T09:44:51Z

libhttp/pico_http_client.c

@@ -2384,7 +2402,7 @@ int8_t pico_http_set_close_ev(uint16_t conn)
 return HTTP_RETURN_ERROR;
 }
 client->wakeup(EV_HTTP_CLOSE, client->connectionID);
-
+ return EV_HTTP_CLOSE;


what's the impact of this line?

Getting rid of warning.

phalox · 2016-10-21T09:46:38Z

test/unit/modunit_pico_http_client.c

+ "UG9vb3dlcjpyYW5nZXI=", "c3VwZXI6cHdk", "aWFtOmdvZA=="};
+
+ int i;
+ for (i = 0; i < 9; i++){


I think there are ways to get rid of the hardcoded 9, but maybe not :-)

phalox · 2016-10-21T09:47:31Z

test/unit/modunit_pico_http_client.c

+ for (i = 0; i < 9; i++){
+ char buffout[256];
+ word32 outLen = sizeof(buffout);
+ Base64_Encode((byte*) userpass[i], strlen(userpass[i]), (byte*) buffout, &outLen);


So here we're actually testing the code from someone else?

phalox · 2016-10-24T07:00:52Z

libhttp/Makefile

@@ -1,4 +1,6 @@
 PICOTCP?=../../picotcp
+WOLFSSL?=../../wolfssl
+OUT?=../../../out


@frederikvs Do we usually get the header files from the out directory ? Although, with the picotcp one it seems that we're also retrieving them from the build directory...

SParmentier · 2016-10-28T11:44:13Z

@phalox Can you review ? I need to add some task to Jira to refactor some functions but now it's working both on our computers and on jenkins.

phalox

Could you change the buffout declaration to a DEFINE lenght? Then you also don't need the sizeof(). Please name this properly so that we know why it's 256 bytes long.

phalox · 2016-10-28T11:53:52Z

libhttp/pico_http_client.c

- // removing the trailing \n from the base46_Encode
- buffout[strlen(buffout)-2] = '\0';
+ // removing the trailing \n from the base46_Encode
+ buffout[strlen(buffout)-1] = '\0';


I'm guessing this was a bug before?

phalox · 2016-10-28T11:54:19Z

libhttp/pico_http_client.c

 if (Base64_Encode((byte*) buffin, (word32) inLen, (byte*) buffout, (word32*) &outLen) != 0){
 // encoding error
 dbg("error happened while encoding\n");
 }
- // removing the trailing \n from the base46_Encode
- buffout[strlen(buffout)-2] = '\0';
+ // removing the trailing \n from the base46_Encode


be careful with spacing. Can you verify if these files were made with tabs or with spaces. Stay consistent.

phalox · 2016-10-28T12:01:40Z

Phew this "pico_process_uri" function got really ugly over time. Could you rework the code? Make smaller functions out of this, get rid of magic numers/calculations of length...

SParmentier · 2016-10-28T12:05:21Z

I'm not really sure to have the time to refactor all the function right now because we'll do a demo now and it's my last afternoon before going to Barco. So I'll make a task on jira ?

phalox · 2016-10-28T12:29:35Z

Sure! :-) Or maybe on github in this repository ( @frederikvs where do you think such a ticket belongs?)
For this it could also be nice that we start following up the quality of the modules.

SParmentier added 15 commits October 12, 2016 12:08

TTQ-792 Add dependencies of wolfssl for compilation

288cb9b

TTQ-792 Remove type warnings

56b89b1

TTQ-792 Add user and password to url

4306a98

TTQ-792 Refactor

54bc437

TTQ-792 Add isdigit test for port

c8c0d8c

TTQ-792 Destroy credentials variable

0712669

TTQ-792 Refactor if else

b6b45a2

TTQ-793 Split client open and process uri test and add more tests to …

70944d5

…process uri

TTQ-793 Add case for wrong format and fix wrong allocation

7f25447

TTQ-993 Fix problem with Base64_Encode

6dbeeb5

TTQ-793 Add unit test for Base64_encoding

98b1038

TTQ-776 forgot the ++ ...

70d95f1

Merge branch 'ci_lamp_dev_clean' of github.com:tass-belgium/picotcp-m…

626fbae

…odules into ci_lamp_dev_clean

TTQ-797 print some informations

88ac686

TTQ-797 Remove one more char in buffout

03df131

phalox suggested changes Oct 21, 2016

View reviewed changes

SParmentier added 2 commits October 21, 2016 13:27

TTQ-797 Use environment var for librairies

e45d279

TTQ-797 Fix memory leak and refactor

b21d199

phalox reviewed Oct 24, 2016

View reviewed changes

TTQ-797 Set buffout to 0 before using it

3118ca8

phalox reviewed Oct 28, 2016

View reviewed changes

Move include .c to unit tests

d7271af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ci lamp dev clean #13

Ci lamp dev clean #13

SParmentier commented Oct 20, 2016

phalox left a comment

phalox Oct 21, 2016

frederikvs Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

frederikvs Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

SParmentier Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 21, 2016

phalox Oct 24, 2016

SParmentier commented Oct 28, 2016

phalox left a comment

phalox Oct 28, 2016

phalox Oct 28, 2016

phalox commented Oct 28, 2016

SParmentier commented Oct 28, 2016

phalox commented Oct 28, 2016

		@@ -378,27 +392,22 @@ static int8_t pico_process_uri(const char uri, struct pico_http_uri urikey)
		index++;

Ci lamp dev clean #13

Are you sure you want to change the base?

Ci lamp dev clean #13

Conversation

SParmentier commented Oct 20, 2016

phalox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SParmentier commented Oct 28, 2016

phalox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phalox commented Oct 28, 2016

SParmentier commented Oct 28, 2016

phalox commented Oct 28, 2016