Coverage #515

outscale-hmi · 2019-12-02T16:27:40Z

No description provided.

outscale-mgo · 2019-12-02T16:34:03Z

Can one of the admins verify this patch?

outscale-mgo

Code is pretty much okay.
Can you reorganise your patches, this PR should contain 4 patches:
2 for the coverage, 1 for the brick modification, and one for moving antispoof define to a public header

outscale-mgo · 2019-12-03T10:19:56Z

tests/antispoof/tests.c

+
+	/* add NDP_MAX adresses  */
+	for (int i = 0; i < 150; i++) {
+		pg_ip_from_str(ip, "2001:db8:2000:aff0::"+i);


"string" + i
will overflow here, and is UB.

outscale-mgo · 2019-12-17T10:25:45Z

tests/core/test-core.c

@@ -44,9 +88,11 @@ static void test_brick_core_simple_lifecycle(void)
 	brick = pg_brick_new("nop", config, &error);
 	g_assert(brick);
 	g_assert(!error);
+	printf("brick refcount is %ld\n",brick->refcount);


no don't add printf please, g_assert the refcount is a good idea though

outscale-mgo · 2019-12-17T10:25:52Z

tests/core/test-core.c


 	pg_brick_decref(brick, &error);
 	g_assert(!error);
+	printf("brick refcount is %ld\n",brick->refcount);


outscale-mgo · 2019-12-17T10:28:53Z

src/brick-int.h

@@ -207,7 +207,7 @@ struct pg_brick *pg_brick_decref(struct pg_brick *brick,
 int pg_brick_reset(struct pg_brick *brick, struct pg_error **errp);

 /* testing */
-uint32_t pg_brick_links_count_get(const struct pg_brick *brick,
+int pg_brick_links_count_get(const struct pg_brick *brick,


this change brick and had nothing to do with coverage, as sure it should be in a different patch

^ this is still valid
Especially for brick which is use everywhere in packetgraph, change in brick deserver a separate commit

outscale-mgo · 2019-12-17T10:29:04Z

src/brick.c

@@ -321,17 +321,17 @@ static uint16_t count_side(const struct pg_brick_side *side,
 * @param	errp a return pointer for an error message
 * @return	the total number of link from the brick to the target
 */
-uint32_t pg_brick_links_count_get(const struct pg_brick *brick,
+int pg_brick_links_count_get(const struct pg_brick *brick,


same for this one

outscale-mgo · 2019-12-17T10:30:04Z

include/packetgraph/antispoof.h

@@ -21,6 +21,9 @@
 #include <packetgraph/common.h>
 #include <packetgraph/errors.h>

+#define PG_ARP_MAX 100
+#define PG_NPD_MAX 100


as this change the brick and not the coverage(and add public define), is should be in another patch

outscale-mgo · 2019-12-17T10:30:33Z

tests/core/test-core.c

@@ -828,6 +828,11 @@ static void test_brick_core_verify_re_link(void)
 	test_brick_sanity_check_expected(f, 0, 0);
 	test_brick_sanity_check_expected(a, 0, 0);

+	pg_brick_unlink(NULL, &e);


Don't seems related to antispoof, maybe it was for the other patch ?

outscale-mgo · 2020-01-07T13:43:32Z

src/brick-int.h

@@ -208,8 +208,8 @@ int pg_brick_reset(struct pg_brick *brick, struct pg_error **errp);

 /* testing */
 uint32_t pg_brick_links_count_get(const struct pg_brick *brick,
-				  const struct pg_brick *target,


I don't think this change is useful

it is just to remove the extra white space

outscale-mgo · 2020-01-07T13:49:11Z

tests/antispoof/test-ndp.c

+/* pkt3 with Next header : UDP (17)
+ *
+ * */
+static unsigned char pkt3[86];


I don't think static is needed

outscale-mgo · 2020-01-07T13:56:59Z

tests/antispoof/tests.c

+		} else {
+			g_assert(pg_antispoof_arp_add(antispoof, i, &error));
+			g_assert(error);
+			pg_error_free(error);


I really think those 3 lines of code could be re-factorise into a macro
(in src/utils/errors.h)

#define PG_CHECK_HAVE_ERROR(error) do {\ g_assert(error);\ pg_error_free(error);\ error = NULL;\ } while (0);

PG_CHECK_HAVE_ERROR is a bad name, but I'm not good at naming

outscale-mgo · 2020-01-07T13:57:23Z

tests/antispoof/tests.c

+	       } else {
+		       g_assert(pg_antispoof_arp_add(antispoof, i, &error));
+		       g_assert(error);
+		       pg_error_free(error);


so you could use the macro here

outscale-mgo · 2020-01-07T14:02:51Z

tests/antispoof/tests.c

@@ -265,8 +316,7 @@ static void test_antispoof_generic(const unsigned char **pkts,
 	pg_brick_unlink(antispoof, &error);
 	g_assert(!error);
 	pg_brick_destroy(antispoof);
-	antispoof = pg_antispoof_new("antispoof", PG_WEST_SIDE,
-				     &inside_mac, &error);
+	antispoof = pg_antispoof_new("antispoof", PG_WEST_SIDE, &inside_mac, &error);


you need to call pg_brick_destroy on antispoof again.

outscale-mgo · 2020-01-07T14:10:54Z

tests/antispoof/tests.c

 	pg_antispoof_ndp_add(antispoof, ip, &error);
 	g_assert(!error);

+	/* remove adresse */
+	g_assert(pg_antispoof_ndp_del(antispoof,ip,&error) == 0);
+        g_assert(!error);


indentation ?

outscale-mgo · 2020-01-07T14:13:13Z

src/brick-int.h

@@ -207,7 +207,7 @@ struct pg_brick *pg_brick_decref(struct pg_brick *brick,
 int pg_brick_reset(struct pg_brick *brick, struct pg_error **errp);

 /* testing */
-uint32_t pg_brick_links_count_get(const struct pg_brick *brick,
+int pg_brick_links_count_get(const struct pg_brick *brick,


^ this is still valid
Especially for brick which is use everywhere in packetgraph, change in brick deserver a separate commit

outscale-mgo · 2020-01-07T14:17:42Z

tests/core/test-core.c

-							     PG_MULTIPOLE);
+	struct pg_brick_config *config = pg_brick_config_new("mybrick", 2, 2, PG_MULTIPOLE);
+	struct pg_brick_config *config1 = pg_brick_config_new("mybrick", 2, 2, PG_MONOPOLE);
+	struct pg_brick_config *config2 = pg_brick_config_new("mybrick", UINT16_MAX, UINT16_MAX, PG_MULTIPOLE);


I think config need to be free, it doesn't seems you free the configs

outscale-mgo · 2020-01-07T14:18:24Z

tests/core/test-core.c

@@ -44,9 +88,11 @@ static void test_brick_core_simple_lifecycle(void)
 	brick = pg_brick_new("nop", config, &error);
 	g_assert(brick);
 	g_assert(!error);
+	printf("brick refcount is %ld\n",brick->refcount);


outscale-mgo · 2020-01-07T14:20:19Z

tests/core/test-core.c

+	middle_brick3 = pg_brick_new("nop", config3, &error);
+	g_assert(!error);
+	east_brick3 = pg_brick_new("nop", config3, &error);
+	g_assert(!error);


I don't think you destroy those brick, you you either need to call pg_brick_ptrptr_destroy at the end of the function or creating the variable using pg_cleanup(pg_brick_ptrptr_destroy)
example:

pg_cleanup(pg_brick_ptrptr_destroy) struct pg_brick_config *config = pg_brick_config_new("mybrick", 2, 2, PG_MULTIPOLE);

also

pg_cleanup(pg_brick_ptrptr_destroy)

is kind of ugly, so maybe we can create a macro

#define pg_autobrick pg_cleanup(pg_brick_ptrptr_destroy)

Signed-off-by: hanen mizouni <[email protected]>

outscale-hmi force-pushed the coverage branch 3 times, most recently from 804729f to 73e15eb Compare December 17, 2019 09:46

outscale-mgo requested changes Dec 17, 2019

View reviewed changes

outscale-hmi force-pushed the coverage branch 4 times, most recently from a58042e to ea8960c Compare January 7, 2020 14:00

outscale-mgo requested changes Jan 7, 2020

View reviewed changes

outscale-hmi force-pushed the coverage branch 8 times, most recently from bee977d to cc9fa12 Compare January 13, 2020 16:35

outscale-hmi added 2 commits January 15, 2020 16:45

test:improve coverage of antispoof

99e6bc6

Signed-off-by: hanen mizouni <[email protected]>

test:improve coverage of brick

b6d0ef5

Signed-off-by: hanen mizouni <[email protected]>

outscale-hmi force-pushed the coverage branch from cc9fa12 to b6d0ef5 Compare January 15, 2020 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coverage #515

Coverage #515

outscale-hmi commented Dec 2, 2019

outscale-mgo commented Dec 2, 2019

outscale-mgo left a comment

outscale-mgo Dec 3, 2019

outscale-mgo Dec 17, 2019

outscale-mgo Jan 7, 2020

outscale-mgo Dec 17, 2019

outscale-mgo Dec 17, 2019

outscale-mgo Jan 7, 2020

outscale-mgo Dec 17, 2019

outscale-mgo Dec 17, 2019

outscale-mgo Dec 17, 2019

outscale-mgo Jan 7, 2020

outscale-hmi Mar 16, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

outscale-hmi Jan 12, 2020

outscale-mgo Jan 7, 2020

outscale-mgo Jan 7, 2020

Coverage #515

Are you sure you want to change the base?

Coverage #515

Conversation

outscale-hmi commented Dec 2, 2019

outscale-mgo commented Dec 2, 2019

outscale-mgo 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

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