2022-03-23 00:24:48

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ v2 4/9] adapter: Don't use DBG in mgmt_debug

From: Luiz Augusto von Dentz <[email protected]>

mgmt_debug callback is used to print debug strings from mgmt instances
which includes the file and function names so using DBG would add yet
another set of file and function prefixes which makes the logs
confusing.
---
src/adapter.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 97ce26f8e..6680c5410 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -10327,9 +10327,8 @@ static void read_version_complete(uint8_t status, uint16_t length,

static void mgmt_debug(const char *str, void *user_data)
{
- const char *prefix = user_data;
-
- info("%s%s", prefix, str);
+ if (DBG_IS_ENABLED())
+ btd_debug(0xffff, "%s", str);
}

int adapter_init(void)
@@ -10342,8 +10341,7 @@ int adapter_init(void)
return -EIO;
}

- if (getenv("MGMT_DEBUG"))
- mgmt_set_debug(mgmt_primary, mgmt_debug, "mgmt: ", NULL);
+ mgmt_set_debug(mgmt_primary, mgmt_debug, NULL, NULL);

DBG("sending read version command");

--
2.35.1

2022-03-24 07:52:33

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ v2 4/9] adapter: Don't use DBG in mgmt_debug

Hi Marcel,

On Wed, Mar 23, 2022 at 3:37 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Luiz,
>
> > mgmt_debug callback is used to print debug strings from mgmt instances
> > which includes the file and function names so using DBG would add yet
> > another set of file and function prefixes which makes the logs
> > confusing.
> > ---
> > src/adapter.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 97ce26f8e..6680c5410 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -10327,9 +10327,8 @@ static void read_version_complete(uint8_t status, uint16_t length,
> >
> > static void mgmt_debug(const char *str, void *user_data)
> > {
> > - const char *prefix = user_data;
> > -
> > - info("%s%s", prefix, str);
> > + if (DBG_IS_ENABLED())
> > + btd_debug(0xffff, "%s", str);
> > }
> >
> > int adapter_init(void)
> > @@ -10342,8 +10341,7 @@ int adapter_init(void)
> > return -EIO;
> > }
> >
> > - if (getenv("MGMT_DEBUG"))
> > - mgmt_set_debug(mgmt_primary, mgmt_debug, "mgmt: ", NULL);
> > + mgmt_set_debug(mgmt_primary, mgmt_debug, NULL, NULL);
>
> oh no. This is crazy. Please re-think this and what computational overhead you are introducing.

I considered moving DBG_IS_ENABLED() in place of getenv("MGMT_DEBUG")
so that would be use just once per adapter, the problem is that
wouldn't work with:

case SIGUSR2:
__btd_toggle_debug();

Or perhaps I just need yet another match that adds the file and
function name so we can strip them out of DBG_IDX.

> Regards
>
> Marcel
>


--
Luiz Augusto von Dentz

2022-03-24 15:27:41

by Marcel Holtmann

Subject: Re: [PATCH BlueZ v2 4/9] adapter: Don't use DBG in mgmt_debug

Hi Luiz,

> mgmt_debug callback is used to print debug strings from mgmt instances
> which includes the file and function names so using DBG would add yet
> another set of file and function prefixes which makes the logs
> confusing.
> ---
> src/adapter.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 97ce26f8e..6680c5410 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -10327,9 +10327,8 @@ static void read_version_complete(uint8_t status, uint16_t length,
>
> static void mgmt_debug(const char *str, void *user_data)
> {
> - const char *prefix = user_data;
> -
> - info("%s%s", prefix, str);
> + if (DBG_IS_ENABLED())
> + btd_debug(0xffff, "%s", str);
> }
>
> int adapter_init(void)
> @@ -10342,8 +10341,7 @@ int adapter_init(void)
> return -EIO;
> }
>
> - if (getenv("MGMT_DEBUG"))
> - mgmt_set_debug(mgmt_primary, mgmt_debug, "mgmt: ", NULL);
> + mgmt_set_debug(mgmt_primary, mgmt_debug, NULL, NULL);

oh no. This is crazy. Please re-think this and what computational overhead you are introducing.

Regards

Marcel

2022-03-25 18:41:34

by Marcel Holtmann

Subject: Re: [PATCH BlueZ v2 4/9] adapter: Don't use DBG in mgmt_debug

Hi Luiz,

>>> mgmt_debug callback is used to print debug strings from mgmt instances
>>> which includes the file and function names so using DBG would add yet
>>> another set of file and function prefixes which makes the logs
>>> confusing.
>>> ---
>>> src/adapter.c | 8 +++-----
>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/adapter.c b/src/adapter.c
>>> index 97ce26f8e..6680c5410 100644
>>> --- a/src/adapter.c
>>> +++ b/src/adapter.c
>>> @@ -10327,9 +10327,8 @@ static void read_version_complete(uint8_t status, uint16_t length,
>>>
>>> static void mgmt_debug(const char *str, void *user_data)
>>> {
>>> - const char *prefix = user_data;
>>> -
>>> - info("%s%s", prefix, str);
>>> + if (DBG_IS_ENABLED())
>>> + btd_debug(0xffff, "%s", str);
>>> }
>>>
>>> int adapter_init(void)
>>> @@ -10342,8 +10341,7 @@ int adapter_init(void)
>>> return -EIO;
>>> }
>>>
>>> - if (getenv("MGMT_DEBUG"))
>>> - mgmt_set_debug(mgmt_primary, mgmt_debug, "mgmt: ", NULL);
>>> + mgmt_set_debug(mgmt_primary, mgmt_debug, NULL, NULL);
>>
>> oh no. This is crazy. Please re-think this and what computational overhead you are introducing.
>
> I considered moving DBG_IS_ENABLED() in place of getenv("MGMT_DEBUG")
> so that would be use just once per adapter, the problem is that
> wouldn't work with:

why do you need this in the first place. The mgmt protocol is also added to btmon traces. Unless you work on src/shared/mgmt.c directly, you don’t need to the debug feature at all. Just let btmon decode it for you.

Regards

Marcel