Multiport linux support by olbjo · Pull Request #303 · rtlabs-com/p-net

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

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

Rebased on latest master

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

Rebased

pyhys

Great!
Mostly trivial changes

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.

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

Review comments fixed

fredrikdanielmoller

Well done!

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

+                                            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.

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

   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

   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

   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


   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

-   /* 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

Comment thread src/pnal.h Outdated

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


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

		handle->if_name = if_name;

Conversation

olbjo commented Jan 15, 2021 • edited by pyhys Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pyhys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olbjo commented Jan 18, 2021

Uh oh!

Uh oh!

olbjo commented Jan 19, 2021

Uh oh!

pyhys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olbjo commented Jan 25, 2021

Uh oh!

fredrikdanielmoller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olbjo commented Jan 26, 2021

Uh oh!

fredrikdanielmoller left a comment

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!

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

olbjo commented Jan 15, 2021 •

edited by pyhys

Loading