You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/10/06 12:29:23 UTC
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull request #418: Incorporate IP configuration files into netinit.
davids5 commented on a change in pull request #418:
URL: https://github.com/apache/incubator-nuttx-apps/pull/418#discussion_r500216142
##########
File path: netutils/netinit/netinit.c
##########
@@ -344,66 +340,239 @@ static void netinit_set_macaddr(void)
#endif
/****************************************************************************
- * Name: netinit_set_ipaddrs
+ * Name: netinit_set_ipv4addrs
*
* Description:
- * Setup IP addresses.
- *
- * For 6LoWPAN, the IP address derives from the MAC address. Setting it
- * to any user provided value is asking for trouble.
+ * Setup IPv4 addresses.
*
****************************************************************************/
#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NET_6LOWPAN) && ! \
- defined(CONFIG_NET_IEEE802154)
-static void netinit_set_ipaddrs(void)
+ defined(CONFIG_NET_IEEE802154) && defined(CONFIG_NET_IPv4)
+static inline void netinit_set_ipv4addrs(void)
{
-#ifdef CONFIG_NET_IPv4
struct in_addr addr;
+#ifdef CONFIG_FSUTILS_IPCFG
+ struct ipv4cfg_s ipv4cfg;
+ int ret;
+#endif
- /* Set up our host address */
+#ifdef CONFIG_FSUTILS_IPCFG
Review comment:
Consider combining the adjacent #ifdefs
##########
File path: netutils/netinit/netinit.c
##########
@@ -344,66 +340,239 @@ static void netinit_set_macaddr(void)
#endif
/****************************************************************************
- * Name: netinit_set_ipaddrs
+ * Name: netinit_set_ipv4addrs
*
* Description:
- * Setup IP addresses.
- *
- * For 6LoWPAN, the IP address derives from the MAC address. Setting it
- * to any user provided value is asking for trouble.
+ * Setup IPv4 addresses.
*
****************************************************************************/
#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NET_6LOWPAN) && ! \
- defined(CONFIG_NET_IEEE802154)
-static void netinit_set_ipaddrs(void)
+ defined(CONFIG_NET_IEEE802154) && defined(CONFIG_NET_IPv4)
+static inline void netinit_set_ipv4addrs(void)
{
-#ifdef CONFIG_NET_IPv4
struct in_addr addr;
+#ifdef CONFIG_FSUTILS_IPCFG
+ struct ipv4cfg_s ipv4cfg;
+ int ret;
+#endif
- /* Set up our host address */
+#ifdef CONFIG_FSUTILS_IPCFG
+ /* Attempt to obtain IPv4 address configuration from the IP configuration
+ * file.
+ */
-#ifndef CONFIG_NETINIT_DHCPC
- addr.s_addr = HTONL(CONFIG_NETINIT_IPADDR);
+ ret = ipcfg_read(NET_DEVNAME, (FAR struct ipcfg_s *)&ipv4cfg, AF_INET);
+#ifdef CONFIG_NETUTILS_DHCPC
+ if (ret >= 0 && ipv4cfg.proto != IPv4PROTO_NONE)
#else
- addr.s_addr = 0;
+ if (ret >= 0 && IPCFG_HAVE_STATIC(ipv4cfg.proto))
#endif
- netlib_set_ipv4addr(NET_DEVNAME, &addr);
+ {
+ /* Check if we are using DHCPC */
- /* Set up the default router address */
+#ifdef CONFIG_NETUTILS_DHCPC
+ if (IPCFG_USE_DHCPC(ipv4cfg.proto))
+ {
+ g_use_dhcpc = true;
+ addr.s_addr = 0;
+ }
+ else
+#endif
+ {
+ /* We are not using DHCPC. We need an IP address */
- addr.s_addr = HTONL(CONFIG_NETINIT_DRIPADDR);
- netlib_set_dripv4addr(NET_DEVNAME, &addr);
+#ifdef CONFIG_NETINIT_IPADDR
+ /* Check if we have a static IP address in the configuration file */
- /* Setup the subnet mask */
+ if (ipv4cfg.proto != 0)
Review comment:
in the DHCP case the `else` limits the scope but would it it make sense to check the static bit here?
##########
File path: netutils/netinit/netinit.c
##########
@@ -344,66 +340,239 @@ static void netinit_set_macaddr(void)
#endif
/****************************************************************************
- * Name: netinit_set_ipaddrs
+ * Name: netinit_set_ipv4addrs
*
* Description:
- * Setup IP addresses.
- *
- * For 6LoWPAN, the IP address derives from the MAC address. Setting it
- * to any user provided value is asking for trouble.
+ * Setup IPv4 addresses.
*
****************************************************************************/
#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NET_6LOWPAN) && ! \
- defined(CONFIG_NET_IEEE802154)
-static void netinit_set_ipaddrs(void)
+ defined(CONFIG_NET_IEEE802154) && defined(CONFIG_NET_IPv4)
+static inline void netinit_set_ipv4addrs(void)
{
-#ifdef CONFIG_NET_IPv4
struct in_addr addr;
+#ifdef CONFIG_FSUTILS_IPCFG
+ struct ipv4cfg_s ipv4cfg;
+ int ret;
+#endif
- /* Set up our host address */
+#ifdef CONFIG_FSUTILS_IPCFG
+ /* Attempt to obtain IPv4 address configuration from the IP configuration
+ * file.
+ */
-#ifndef CONFIG_NETINIT_DHCPC
- addr.s_addr = HTONL(CONFIG_NETINIT_IPADDR);
+ ret = ipcfg_read(NET_DEVNAME, (FAR struct ipcfg_s *)&ipv4cfg, AF_INET);
+#ifdef CONFIG_NETUTILS_DHCPC
+ if (ret >= 0 && ipv4cfg.proto != IPv4PROTO_NONE)
Review comment:
Side comment: Is ipcfg_read checking for erased values of 0xff and the to come build version vs read version.?
##########
File path: netutils/netinit/netinit.c
##########
@@ -344,66 +340,239 @@ static void netinit_set_macaddr(void)
#endif
/****************************************************************************
- * Name: netinit_set_ipaddrs
+ * Name: netinit_set_ipv4addrs
*
* Description:
- * Setup IP addresses.
- *
- * For 6LoWPAN, the IP address derives from the MAC address. Setting it
- * to any user provided value is asking for trouble.
+ * Setup IPv4 addresses.
*
****************************************************************************/
#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NET_6LOWPAN) && ! \
- defined(CONFIG_NET_IEEE802154)
-static void netinit_set_ipaddrs(void)
+ defined(CONFIG_NET_IEEE802154) && defined(CONFIG_NET_IPv4)
+static inline void netinit_set_ipv4addrs(void)
{
-#ifdef CONFIG_NET_IPv4
struct in_addr addr;
+#ifdef CONFIG_FSUTILS_IPCFG
+ struct ipv4cfg_s ipv4cfg;
+ int ret;
+#endif
- /* Set up our host address */
+#ifdef CONFIG_FSUTILS_IPCFG
+ /* Attempt to obtain IPv4 address configuration from the IP configuration
+ * file.
+ */
-#ifndef CONFIG_NETINIT_DHCPC
- addr.s_addr = HTONL(CONFIG_NETINIT_IPADDR);
+ ret = ipcfg_read(NET_DEVNAME, (FAR struct ipcfg_s *)&ipv4cfg, AF_INET);
+#ifdef CONFIG_NETUTILS_DHCPC
+ if (ret >= 0 && ipv4cfg.proto != IPv4PROTO_NONE)
#else
- addr.s_addr = 0;
+ if (ret >= 0 && IPCFG_HAVE_STATIC(ipv4cfg.proto))
#endif
- netlib_set_ipv4addr(NET_DEVNAME, &addr);
+ {
+ /* Check if we are using DHCPC */
- /* Set up the default router address */
+#ifdef CONFIG_NETUTILS_DHCPC
+ if (IPCFG_USE_DHCPC(ipv4cfg.proto))
+ {
+ g_use_dhcpc = true;
+ addr.s_addr = 0;
+ }
+ else
+#endif
+ {
+ /* We are not using DHCPC. We need an IP address */
- addr.s_addr = HTONL(CONFIG_NETINIT_DRIPADDR);
- netlib_set_dripv4addr(NET_DEVNAME, &addr);
+#ifdef CONFIG_NETINIT_IPADDR
+ /* Check if we have a static IP address in the configuration file */
- /* Setup the subnet mask */
+ if (ipv4cfg.proto != 0)
+ {
+ addr.s_addr = ipv4cfg.ipaddr;
+ }
+ else
+ {
+ addr.s_addr = HTONL(CONFIG_NETINIT_IPADDR);
+ }
+#else
+ /* Use whatever was provided in the file (might be zero) */
- addr.s_addr = HTONL(CONFIG_NETINIT_NETMASK);
- netlib_set_ipv4netmask(NET_DEVNAME, &addr);
+ addr.s_addr = ipv4cfg.ipaddr;
#endif
+ }
-#ifdef CONFIG_NET_IPv6
+ netlib_set_ipv4addr(NET_DEVNAME, &addr);
+
+ /* Set up the remaining addresses */
+
+ if (IPCFG_HAVE_STATIC(ipv4cfg.proto))
+ {
+ /* Set up the default router address */
+
+ addr.s_addr = ipv4cfg.router;
+ netlib_set_dripv4addr(NET_DEVNAME, &addr);
+
+ /* Setup the subnet mask */
+
+ addr.s_addr = ipv4cfg.netmask;
+ netlib_set_ipv4netmask(NET_DEVNAME, &addr);
+ }
+
+#ifdef CONFIG_NETUTILS_DHCPC
+ /* No static addresses? That is fine if we are have addresses
+ * provided by the configuration, or if we are using DHCP.
+ */
+
+ else if (g_use_dhcpc)
+ {
+ /* Set up the default router address and sub-net mask */
+
+ addr.s_addr = 0;
+ netlib_set_dripv4addr(NET_DEVNAME, &addr);
+ netlib_set_ipv4netmask(NET_DEVNAME, &addr);
+ }
+#endif
+ else
+ {
+ /* Otherwise, set up the configured default router address */
+
+ addr.s_addr = HTONL(CONFIG_NETINIT_DRIPADDR);
Review comment:
This case seems redundant to the one on line 411. But I am having a super hard time with all the #ifdef and 'else' and maintaining visual alignment. - that might just be my deficit. Which is fine. But I cannot help I wonder if it would be better from a readability stand point to assemble a state variable and have switch statement ?
----------------------------------------------------------------
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