[PATCH v3 1/2] netconfig: Add multiple levels to IWD_DHCP_DEBUG

[PATCH v3 1/2] netconfig: Add multiple levels to IWD_DHCP_DEBUG

[Michael Johnson]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

Allows granularly specifying the DHCP logging level. This allows the
user to tailor the output to what they need. By default, always display
info, errors and warnings to match the rest of iwd.

Setting `IWD_DHCP_DEBUG` to "debug", "info", "warn", "error" will limit
the logging to that level or higher allowing the default logging
verbosity to be reduced.

Setting `IWD_DHCP_DEBUG` to "1" as per the current behavior will
continue to enable debug level logging.
---
 src/netconfig.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/netconfig.c b/src/netconfig.c
index 6694a0e9..0042a55d 100644
--- a/src/netconfig.c
+++ b/src/netconfig.c
@@ -1621,6 +1621,8 @@ struct netconfig *netconfig_new(uint32_t ifindex)
 	struct netdev *netdev = netdev_find(ifindex);
 	struct netconfig *netconfig;
 	struct l_icmp6_client *icmp6;
+	const char *debug_level = NULL;
+	int dhcp_priority = L_LOG_INFO;
 
 	if (!netconfig_list)
 		return NULL;
@@ -1640,9 +1642,22 @@ struct netconfig *netconfig_new(uint32_t ifindex)
 					netconfig_ipv4_dhcp_event_handler,
 					netconfig, NULL);
 
-	if (getenv("IWD_DHCP_DEBUG"))
-		l_dhcp_client_set_debug(netconfig->dhcp_client, do_debug,
-							"[DHCPv4] ", NULL);
+	debug_level = getenv("IWD_DHCP_DEBUG");
+	if (debug_level != NULL) {
+		if (strcmp("debug", debug_level) == 0)
+			dhcp_priority = L_LOG_DEBUG;
+		else if (strcmp("info", debug_level) == 0)
+			dhcp_priority = L_LOG_INFO;
+		else if (strcmp("warn", debug_level) == 0)
+			dhcp_priority = L_LOG_WARNING;
+		else if (strcmp("error", debug_level) == 0)
+			dhcp_priority = L_LOG_ERR;
+		else 	/* Default for backwards compatibility */
+			dhcp_priority = L_LOG_DEBUG;
+	}
+
+	l_dhcp_client_set_debug(netconfig->dhcp_client, do_debug,
+					"[DHCPv4] ", NULL, dhcp_priority);
 
 	netconfig->dhcp6_client = l_dhcp6_client_new(ifindex);
 	l_dhcp6_client_set_event_handler(netconfig->dhcp6_client,
-- 
2.25.1

Re: [PATCH v3 1/2] netconfig: Add multiple levels to IWD_DHCP_DEBUG

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

Hi Michael,

On 5/18/22 09:39, Michael Johnson wrote:
> Allows granularly specifying the DHCP logging level. This allows the
> user to tailor the output to what they need. By default, always display
> info, errors and warnings to match the rest of iwd.
> 
> Setting `IWD_DHCP_DEBUG` to "debug", "info", "warn", "error" will limit
> the logging to that level or higher allowing the default logging
> verbosity to be reduced.
> 
> Setting `IWD_DHCP_DEBUG` to "1" as per the current behavior will
> continue to enable debug level logging.
> ---
>   src/netconfig.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 

<snip>

> +		if (strcmp("debug", debug_level) == 0)

We prefer !strcmp instead of comparing with 0, so I amended this.

> +			dhcp_priority = L_LOG_DEBUG;
> +		else if (strcmp("info", debug_level) == 0)
> +			dhcp_priority = L_LOG_INFO;
> +		else if (strcmp("warn", debug_level) == 0)
> +			dhcp_priority = L_LOG_WARNING;
> +		else if (strcmp("error", debug_level) == 0)
> +			dhcp_priority = L_LOG_ERR;
> +		else 	/* Default for backwards compatibility */

And I think there was a stray whitespace before <tab> that I took care of as well.

> +			dhcp_priority = L_LOG_DEBUG;
> +	}
> +

Both patches applied, thanks!

Regards,
-Denis

Re: [PATCH v3 1/2] netconfig: Add multiple levels to IWD_DHCP_DEBUG

[Michael Johnson]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

Hi Marcel,

On Wed, 18 May 2022 at 15:48, Marcel Holtmann <marcel(a)holtmann.org> wrote:

> > +     l_dhcp_client_set_debug(netconfig->dhcp_client, do_debug,
> > +                                     "[DHCPv4] ", NULL, dhcp_priority);
>
> please don’t always set l_dhcp_client_set_debug. It should not be set. And I don’t like the dhcp_priority parameter at all. That is just convoluted code and there is no precedence with any _set_debug() APIs.

Just so I'm on the same page, my intention is to turn on logging with
the DHCP client by default so warnings and errors aren't silently
dropped. Ideally I would also like to have a certain level of info
logging that matches other DHCP clients so it's possible to see what
state the system is in. Maybe this isn't something iwd is interested
in, which is fine, but if it is, what would you propose I do instead
here?

Regards,
Michael

Re: [PATCH v3 1/2] netconfig: Add multiple levels to IWD_DHCP_DEBUG

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

Hi Michael,

> Allows granularly specifying the DHCP logging level. This allows the
> user to tailor the output to what they need. By default, always display
> info, errors and warnings to match the rest of iwd.
> 
> Setting `IWD_DHCP_DEBUG` to "debug", "info", "warn", "error" will limit
> the logging to that level or higher allowing the default logging
> verbosity to be reduced.
> 
> Setting `IWD_DHCP_DEBUG` to "1" as per the current behavior will
> continue to enable debug level logging.
> ---
> src/netconfig.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/netconfig.c b/src/netconfig.c
> index 6694a0e9..0042a55d 100644
> --- a/src/netconfig.c
> +++ b/src/netconfig.c
> @@ -1621,6 +1621,8 @@ struct netconfig *netconfig_new(uint32_t ifindex)
> 	struct netdev *netdev = netdev_find(ifindex);
> 	struct netconfig *netconfig;
> 	struct l_icmp6_client *icmp6;
> +	const char *debug_level = NULL;
> +	int dhcp_priority = L_LOG_INFO;
> 
> 	if (!netconfig_list)
> 		return NULL;
> @@ -1640,9 +1642,22 @@ struct netconfig *netconfig_new(uint32_t ifindex)
> 					netconfig_ipv4_dhcp_event_handler,
> 					netconfig, NULL);
> 
> -	if (getenv("IWD_DHCP_DEBUG"))
> -		l_dhcp_client_set_debug(netconfig->dhcp_client, do_debug,
> -							"[DHCPv4] ", NULL);
> +	debug_level = getenv("IWD_DHCP_DEBUG");
> +	if (debug_level != NULL) {
> +		if (strcmp("debug", debug_level) == 0)
> +			dhcp_priority = L_LOG_DEBUG;
> +		else if (strcmp("info", debug_level) == 0)
> +			dhcp_priority = L_LOG_INFO;
> +		else if (strcmp("warn", debug_level) == 0)
> +			dhcp_priority = L_LOG_WARNING;
> +		else if (strcmp("error", debug_level) == 0)
> +			dhcp_priority = L_LOG_ERR;
> +		else 	/* Default for backwards compatibility */
> +			dhcp_priority = L_LOG_DEBUG;
> +	}
> +
> +	l_dhcp_client_set_debug(netconfig->dhcp_client, do_debug,
> +					"[DHCPv4] ", NULL, dhcp_priority);

please don’t always set l_dhcp_client_set_debug. It should not be set. And I don’t like the dhcp_priority parameter at all. That is just convoluted code and there is no precedence with any _set_debug() APIs.

Regards

Marcel