Multiport linux support #303

olbjo · 2021-01-15T15:00:14Z

See doc/multiple_ports.rst for information on how to run configure
and run a mulitport configuration
Verified on Raspberry Pi with additional USB ethernet interfaces

pyhys

Great work!
I think we need to enable filtering of incoming packets dependent on Ethertype already in the Linux kernel, otherwise it will be lots of work for the application. See os_eth_init() in https://github.com/rtlabs-com/p-net/blob/6670292041fd85c028ed1d40c22d5b1ced2fd882/src/osal/linux/osal_eth.c

doc/multiple_ports.rst

sample_app/sampleapp_common.h

sample_app/sampleapp_common.c

src/common/pf_eth.c

src/common/pf_lldp.h

src/common/pf_eth.c

src/ports/rt-kernel/pnal_eth.c

olbjo · 2021-01-18T10:04:25Z

Rebased on latest master

src/pnal.h

olbjo · 2021-01-19T13:04:12Z

Rebased

pyhys

Great!
Mostly trivial changes

sample_app/sampleapp_common.c

sample_app/sampleapp_common.h

src/ports/linux/sampleapp_main.c

src/ports/rt-kernel/pnal_sys.h

src/ports/rt-kernel/sampleapp_main.c

src/device/pf_port.c

src/device/pnet_api.c

src/common/pf_eth.h

src/pnal.h

src/common/pf_lldp.c

src/pnal.h

src/common/pf_lldp.c

src/common/pf_lldp.h

src/device/pf_port.h

src/common/pf_eth.h

olbjo · 2021-01-25T12:31:38Z

Review comments fixed.
I have also run regression tests on a raspberry pi with a single port configuration without seeing any problems. (Not all tests in suite run).

fredrikdanielmoller

Reviewed changes for Ethernet, LLDP and port handling. Didn't look at code for sample app and unit tests.

sample_app/sampleapp_common.c

src/common/pf_eth.c

src/common/pf_eth.h

src/common/pf_lldp.h

src/device/pf_port.h

src/device/pnet_api.c

src/pnal.h

src/ports/rt-kernel/pnal_eth.c

olbjo · 2021-01-26T12:58:13Z

Review comments fixed

fredrikdanielmoller

Well done!

hefloryd · 2021-01-29T13:58:48Z

sample_app/sampleapp_common.c

+                                            for test. */
+   char c;
+
+   memset (p_if_list, 0, sizeof (*p_if_list));


Would it be simpler to use strtok(_r)? here instead? Something along the lines of:

char * argv[16]; char * p; char * saveptr; /* Split line into argv using space as delimiter */ p = strtok_r (line, " ", &saveptr); while (p != NULL && argc < NELEMENTS (argv) - 1) { argv[argc++] = p; p = strtok_r (NULL, " ", &saveptr); }

Yes, possibly. My first idea was to use strtok, but then I needed to copy arg_str for strtok to be able to insert '\0' and I also need to copy the strings so I ended up with this implementation.

Do you prefer that I use a strtok-based implementation instead?

When possible we should use the standard libraries. Unfortunately strok is not reentrant and strok_r (which is reentrant) might not be portable (it's not available on windows for instance). So let's stick with this for now.

This network initialization / configuration from the application is messy. We will try to improvement according to #316. In this case this code may disappear.

sample_app/sampleapp_common.h

hefloryd · 2021-01-29T14:43:31Z

test/mocks.cpp

@@ -57,8 +58,9 @@ pnal_eth_handle_t * mock_pnal_eth_init (
 {
   pnal_eth_handle_t * handle;

-   handle = (pnal_eth_handle_t *)calloc (1, sizeof (pnal_eth_handle_t));
+   handle = (pnal_eth_handle_t *)calloc (1, 10 /*sizeof (pnal_eth_handle_t)*/);


Was this intended?

I agree this looks strange. See updated mock implementation.

hefloryd · 2021-01-29T14:46:29Z

test/mocks.cpp

@@ -103,6 +105,11 @@ int mock_pnal_eth_send (pnal_eth_handle_t * handle, pnal_buf_t * p_buf)
   return p_buf->len;
 }

+int mock_pnal_eth_recv (void * arg, pnal_buf_t * buf)


This seems backwards? The purpose of a mock-function is to decouple from external dependencies. This instead adds an external dependency?

I have updated the mock implementation.
Done

hefloryd · 2021-01-29T14:47:08Z

test/test_dcp.cpp

@@ -101,10 +101,10 @@ TEST_F (DcpTest, DcpHelloTest)

   p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE);
   memcpy (p_buf->payload, get_name_req, sizeof (get_name_req));
-   ret = pf_eth_recv (net, p_buf);
+   ret = mock_pnal_eth_recv (net, p_buf);


What is being tested here? Looks like it is a test of the mock-function.

I have updated the the mock implementation to avoid this.
Done

olbjo · 2021-02-01T10:17:45Z

Rebased branch on master

fredrikdanielmoller · 2021-02-01T10:45:38Z

test/mocks.cpp

@@ -59,6 +66,10 @@ pnal_eth_handle_t * mock_pnal_eth_init (

   handle = (pnal_eth_handle_t *)calloc (1, sizeof (pnal_eth_handle_t));

+   handle->if_name = if_name;


Instead of allocating a pnal_eth_handle_t object in this function, perhaps one could initialise mock_os_data.eth_if_handle in mock_init() to some constant pointer, e.g (void*)42, and then return it here?

Agreed. BTW, doesn't the calloc call leak memory for every test-case using mock_clear()?

Yes, I assume so. I will update according to Fredriks idea.

hefloryd · 2021-02-02T09:48:28Z

src/device/pnet_api.c

-   /* Initialize everything (and the DCP protocol) */
-   /* First initialize the network interface */
-   net->eth_handle =
-      pnal_eth_init (p_cfg->if_cfg.main_port.if_name, pf_eth_recv, (void *)net);


Remove #define pnal_eth_init mock_pnal_eth_init also

Done, second push below is a rebase on master

- See doc/multiple_ports.rst for information on how to configure and run a mulitport configuration - Verified on Raspberry Pi with additional USB ethernet interfaces

olbjo requested a review from pyhys January 15, 2021 15:00

pyhys reviewed Jan 18, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from 207a56b to d904487 Compare January 18, 2021 10:03

fredrikdanielmoller reviewed Jan 18, 2021

View reviewed changes

src/pnal.h Outdated Show resolved Hide resolved

olbjo force-pushed the multi_port_linux branch from d904487 to 548fd5e Compare January 19, 2021 12:35

olbjo force-pushed the multi_port_linux branch from 548fd5e to c9e0ad4 Compare January 19, 2021 16:47

pyhys reviewed Jan 20, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from c9e0ad4 to 6a56f03 Compare January 20, 2021 15:03

pyhys approved these changes Jan 20, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from 6a56f03 to 29654d3 Compare January 20, 2021 15:40

fredrikdanielmoller suggested changes Jan 21, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from 29654d3 to a0ff3a9 Compare January 25, 2021 09:00

fredrikdanielmoller suggested changes Jan 26, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from a0ff3a9 to f1e70e3 Compare January 26, 2021 12:57

olbjo force-pushed the multi_port_linux branch from f1e70e3 to 60262b9 Compare January 26, 2021 13:03

fredrikdanielmoller approved these changes Jan 26, 2021

View reviewed changes

pyhys requested a review from hefloryd January 26, 2021 13:52

olbjo force-pushed the multi_port_linux branch from 60262b9 to 65f6a41 Compare January 27, 2021 13:25

olbjo mentioned this pull request Jan 27, 2021

Multi port updated #318

Merged

hefloryd reviewed Jan 29, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch 2 times, most recently from 3a8e838 to f1de47b Compare February 1, 2021 10:17

fredrikdanielmoller reviewed Feb 1, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch 2 times, most recently from 1f5358b to 6ce2de1 Compare February 1, 2021 12:04

hefloryd approved these changes Feb 2, 2021

View reviewed changes

olbjo force-pushed the multi_port_linux branch from 6ce2de1 to fd69be0 Compare February 2, 2021 10:13

Multiport linux support

c48c415

- See doc/multiple_ports.rst for information on how to configure and run a mulitport configuration - Verified on Raspberry Pi with additional USB ethernet interfaces

olbjo force-pushed the multi_port_linux branch from fd69be0 to c48c415 Compare February 2, 2021 10:15

hefloryd merged commit fced618 into rtlabs-com:master Feb 2, 2021

olbjo deleted the multi_port_linux branch May 19, 2021 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiport linux support #303

Multiport linux support #303

olbjo commented Jan 15, 2021 •

edited by pyhys

Loading

pyhys left a comment

olbjo commented Jan 18, 2021

olbjo commented Jan 19, 2021

pyhys left a comment

olbjo commented Jan 25, 2021

fredrikdanielmoller left a comment

olbjo commented Jan 26, 2021

fredrikdanielmoller left a comment

hefloryd Jan 29, 2021

olbjo Feb 1, 2021

hefloryd Feb 1, 2021

olbjo Feb 1, 2021

hefloryd Jan 29, 2021

olbjo Feb 1, 2021

hefloryd Jan 29, 2021

olbjo Feb 1, 2021

hefloryd Jan 29, 2021

olbjo Feb 1, 2021

olbjo commented Feb 1, 2021

fredrikdanielmoller Feb 1, 2021

hefloryd Feb 1, 2021

olbjo Feb 1, 2021

hefloryd Feb 2, 2021

olbjo Feb 2, 2021

		@@ -59,6 +66,10 @@ pnal_eth_handle_t * mock_pnal_eth_init (

		handle = (pnal_eth_handle_t *)calloc (1, sizeof (pnal_eth_handle_t));

		handle->if_name = if_name;

Multiport linux support #303

Multiport linux support #303

Conversation

olbjo commented Jan 15, 2021 • edited by pyhys Loading

pyhys left a comment

Choose a reason for hiding this comment

olbjo commented Jan 18, 2021

olbjo commented Jan 19, 2021

pyhys left a comment

Choose a reason for hiding this comment

olbjo commented Jan 25, 2021

fredrikdanielmoller left a comment

Choose a reason for hiding this comment

olbjo commented Jan 26, 2021

fredrikdanielmoller 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

olbjo commented Feb 1, 2021

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

olbjo commented Jan 15, 2021 •

edited by pyhys

Loading