[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1886: Allows network monitoring (apps repo) using polling and allows the board to provide the IP config at runtime.

[GitBox <gi...@apache.org>]

patacongo commented on a change in pull request #1886:
URL: https://github.com/apache/incubator-nuttx/pull/1886#discussion_r495024947



##########
File path: include/sys/boardctl.h
##########
@@ -253,6 +261,26 @@ struct boardioc_pm_ctrl_s
 };
 #endif
 
+#ifdef CONFIG_BOARDCTL_NETCONF
+/* Describes the network configuration */
+
+enum boardioc_netconf_e
+{
+  BOARDIOC_NETCONF_DHCP     = 0x1, /* Use DHCP */
+  BOARDIOC_NETCONF_STATIC   = 0x2, /* Use static IP */
+  BOARDIOC_NETCONF_FALLBACK = 0x3, /* Use DHCP with fall back static IP */
+};
+
+struct boardioc_netconf_s
+{
+  enum boardioc_netconf_e flags; /* Configure for static and/or DHCP */
+  uint32_t ipaddr;
+  uint32_t netmask;
+  uint32_t dnsaddr;
+  uint32_t default_router;
+};
+#endif
+

Review comment:
       Most of this is already available in procfs/net/<dev>.  See:
   
       fs/procfs/fs_procfs.c
       net/procfs/
   
   This is where the NSH ifconfig command gets its network information.
   
   This addition is too redundant and adds no value.  In fact, I say that it degrades the OS overall.  Let's not merge it.

##########
File path: boards/Kconfig
##########
@@ -3118,6 +3118,14 @@ config BOARDCTL_UNIQUEID_SIZE
 		Provides the size of the memory buffer that must be provided by the
 		caller of board_uniqueid() in which to receive the board unique ID.
 
+config BOARDCTL_NETCONF
+	bool "Return board network configuration"
+	default n
+	---help---
+		Enables support for the BOARDIOC_NETCONF boardctl() command.
+		Architecture specific logic must provide the board_get_netconf()
+		interface.
+

Review comment:
       Wouldn't the procfs be a better, more standard way to get configuration information?  boardctl() is a necessary kludge, but we should really use it as little as possible since it is inherently non-POSIX, non-standard.  procfs is the recommended way.
   
   The preferred way would be to support a procfs folder with files named eth0, eth1, etc.  Reading any one of this files would return the configuration of that network.
   
   Is there any precedence for anything like this in Linux?

##########
File path: include/nuttx/board.h
##########
@@ -274,6 +274,28 @@ int board_power_off(int status);
 int board_reset(int status);
 #endif
 
+/****************************************************************************
+ * Name: board_get_netconf
+ *
+ * Description:
+ *   Populates the values if the boardioc_netconf_s to configure the
+ *   network.
+ *
+ *
+ * Input Parameters:
+ *   netconf - A reference to a writable memory location provided by the
+ *     caller to receive the board boardioc_netconf_s.
+ *
+ * Returned Value:
+ *   Zero (OK) is returned on success.  Otherwise a negated errno value is
+ *   returned indicating the nature of the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_NETCONF
+int board_get_netconf(struct boardioc_netconf_s *netconf);

Review comment:
       This needs to be extended to handle multiple network devices.  eth0, eth1, etc.  Different network devices will be configured differently.  Apparently, this implementation consider only, what?, eth0?  This needs to be extended.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org