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

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

[Michael Johnson]

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

Hi Denis,

On Tue, 17 May 2022 at 19:09, Denis Kenzior <denkenz(a)gmail.com> wrote:
...
>
> Actually, that's not our coding style.  We use Linux Kernel style, so it would be:

Sorry about that, I'll fix it up. I saw the functions and assumed
without checking other `if`'s.

> This changes the semantics slightly.  In our wiki, etc we show IWD_DHCP_DEBUG
> with value "1" to enable debugging, so I think we should be nice and keep that
> behavior if possible.  How about:
>
> if IWD_DHCP_DEBUG is not set, use L_LOG_WARNING.
>
> If IWD_DHCP_DEBUG is set and has a non-NULL value, use L_LOG_DEBUG
>
> If the value above is from the set L_LOG_ERR .. L_LOG_DEBUG (3 .. 7), then use that.

I made this change but it seems quite difficult to explain in a
reasonable way to the user why the values are 3, 4, 6, and 7.
How about using strings instead? `info`, `warn`, `error`, `debug`?
That still allows us to keep the old behavior if it's a `1`.

I'm also questioning if I should keep the L_LOG_WARN as the default or
make it L_LOG_INFO which is more in line with what I intended when I
started (i.e. not setting the env should result in similar output to
other DHCP clients). Do you prefer WARN?

Regards,
Michael

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

[Michael Johnson]

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

Hi Denis,

On Wed, 18 May 2022 at 15:06, Denis Kenzior <denkenz(a)gmail.com> wrote:

> Send it out and I'll ponder which one I like more.  I think longer term, it
> might be desirable to be able to quiet down some of this output, even for iwd
> itself.

Ok, I've sent it out.

I also sent another ell patch for changes that would allow limiting
the iwd logging as well which I wrote when putting together a more
complex patch (before it got too complex). This would go better with
the second iwd patch as the verbosity can be reduced through a new
`l_log_set_max_level` function in ell/log and therefore IWD_DHCP_DEBUG
can remain simple.

I don't have a matching patch to limit logging verbosity in all of iwd
though it should be fairly straightforward to add.

Regards,
Michael

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

[Denis Kenzior]

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

Hi Michael,

On 5/18/22 07:36, Michael Johnson wrote:
> Hi Denis,
> 
> On Tue, 17 May 2022 at 20:23, Denis Kenzior <denkenz(a)gmail.com> wrote:
>> If you intended this to be info, then do that.  iwd prints l_info, l_error and
>> l_warn messages by default and only l_debug is enabled via '-d'.
> 
> This makes me think DHCP should just follow the same pattern for
> simplicity. I have sent a new patch which always enable DHCP info
> level logging and if the user sets IWD_DHCP_DEBUG then bump it up to
> debug.
> This patch is much simpler and doesn't change the behavior of the env
> var but it comes with the downside of not being able to quieten it
> (although that is similar to iwd).
> 
> I have the v3 version of this patch ready with the string comparisons
> if you prefer the first approach.

Send it out and I'll ponder which one I like more.  I think longer term, it 
might be desirable to be able to quiet down some of this output, even for iwd 
itself.

Regards,
-Denis

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

[Michael Johnson]

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

Hi Denis,

On Tue, 17 May 2022 at 20:23, Denis Kenzior <denkenz(a)gmail.com> wrote:
> If you intended this to be info, then do that.  iwd prints l_info, l_error and
> l_warn messages by default and only l_debug is enabled via '-d'.

This makes me think DHCP should just follow the same pattern for
simplicity. I have sent a new patch which always enable DHCP info
level logging and if the user sets IWD_DHCP_DEBUG then bump it up to
debug.
This patch is much simpler and doesn't change the behavior of the env
var but it comes with the downside of not being able to quieten it
(although that is similar to iwd).

I have the v3 version of this patch ready with the string comparisons
if you prefer the first approach.

Regards,
Michael

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

[Denis Kenzior]

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

Hi Michael,

>> This changes the semantics slightly.  In our wiki, etc we show IWD_DHCP_DEBUG
>> with value "1" to enable debugging, so I think we should be nice and keep that
>> behavior if possible.  How about:
>>
>> if IWD_DHCP_DEBUG is not set, use L_LOG_WARNING.
>>
>> If IWD_DHCP_DEBUG is set and has a non-NULL value, use L_LOG_DEBUG
>>
>> If the value above is from the set L_LOG_ERR .. L_LOG_DEBUG (3 .. 7), then use that.
> 
> I made this change but it seems quite difficult to explain in a
> reasonable way to the user why the values are 3, 4, 6, and 7.
> How about using strings instead? `info`, `warn`, `error`, `debug`?
> That still allows us to keep the old behavior if it's a `1`.

Sure, that sounds even better.

> 
> I'm also questioning if I should keep the L_LOG_WARN as the default or
> make it L_LOG_INFO which is more in line with what I intended when I
> started (i.e. not setting the env should result in similar output to
> other DHCP clients). Do you prefer WARN?

If you intended this to be info, then do that.  iwd prints l_info, l_error and 
l_warn messages by default and only l_debug is enabled via '-d'.

Regards,
-Denis

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

[Denis Kenzior]

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

Hi Michael,

On 5/17/22 12:16, 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
> errors and warnings.
> 
> 1 = INFO
> 2 = DEBUG
> ---
>   src/netconfig.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/src/netconfig.c b/src/netconfig.c
> index 6694a0e9..da9969f2 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_WARNING;
>   
>   	if (!netconfig_list)
>   		return NULL;
> @@ -1640,9 +1642,18 @@ 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)
> +	{
> +		// Convert from user 1 or 2 to L_LOG_*

Actually, that's not our coding style.  We use Linux Kernel style, so it would be:

if (debug_level != NULL) {

and no C++ comments please.

> +		if (*debug_level == '1')
> +			dhcp_priority = L_LOG_INFO;
> +		else if (*debug_level == '2')
> +			dhcp_priority = L_LOG_DEBUG;
> +	}
> +

This changes the semantics slightly.  In our wiki, etc we show IWD_DHCP_DEBUG 
with value "1" to enable debugging, so I think we should be nice and keep that 
behavior if possible.  How about:

if IWD_DHCP_DEBUG is not set, use L_LOG_WARNING.

If IWD_DHCP_DEBUG is set and has a non-NULL value, use L_LOG_DEBUG

If the value above is from the set L_LOG_ERR .. L_LOG_DEBUG (3 .. 7), then use that.

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

Regards,
-Denis

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

[Michael Johnson]

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

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

1 = INFO
2 = DEBUG
---
 src/netconfig.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/netconfig.c b/src/netconfig.c
index 6694a0e9..da9969f2 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_WARNING;
 
 	if (!netconfig_list)
 		return NULL;
@@ -1640,9 +1642,18 @@ 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)
+	{
+		// Convert from user 1 or 2 to L_LOG_*
+		if (*debug_level == '1')
+			dhcp_priority = L_LOG_INFO;
+		else if (*debug_level == '2')
+			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