* [PATCH bluez] bnep: don't error() if kernel lacks bnep module
@ 2013-09-06 14:23 David Herrmann
2013-09-13 10:01 ` Johan Hedberg
0 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2013-09-06 14:23 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Johan Hedberg, David Herrmann
If a user disables bnep kernel support, they normally do that on purpose.
It's misleading to print an error during module-startup in this situation.
Therefore, only print a hint that bnep-support is missing if
EPROTONOSUPPORT is returned by the kernel.
Furthermore, allow modules to forward ENOSYS as error to bluetoothd core
to handle it as "module is not compatible with system setup" instead of an
error. ENOSYS is commonly used to signal "missing kernel infrastructure"
so it seems appropriate here.
---
profiles/network/common.c | 7 ++++++-
profiles/network/manager.c | 17 ++++++++++++++---
src/plugin.c | 11 +++++++++--
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/profiles/network/common.c b/profiles/network/common.c
index e069892..17bff30 100644
--- a/profiles/network/common.c
+++ b/profiles/network/common.c
@@ -110,8 +110,13 @@ int bnep_init(void)
if (ctl < 0) {
int err = -errno;
- error("Failed to open control socket: %s (%d)",
+
+ if (err == -EPROTONOSUPPORT)
+ info("kernel lacks bnep-protocol support");
+ else
+ error("Failed to open control socket: %s (%d)",
strerror(-err), -err);
+
return err;
}
diff --git a/profiles/network/manager.c b/profiles/network/manager.c
index 03b1b3d..7697766 100644
--- a/profiles/network/manager.c
+++ b/profiles/network/manager.c
@@ -25,6 +25,7 @@
#include <config.h>
#endif
+#include <errno.h>
#include <stdbool.h>
#include <bluetooth/bluetooth.h>
@@ -169,11 +170,21 @@ static struct btd_profile nap_profile = {
static int network_init(void)
{
+ int r;
+
read_config(CONFIGDIR "/network.conf");
- if (bnep_init()) {
- error("Can't init bnep module");
- return -1;
+ r = bnep_init();
+ if (r) {
+ if (r == -EPROTONOSUPPORT) {
+ info("bnep module not available, disabling plugin");
+ r = -ENOSYS;
+ } else {
+ error("Can't init bnep module");
+ r = -1;
+ }
+
+ return r;
}
/*
diff --git a/src/plugin.c b/src/plugin.c
index 51c98bc..396c8af 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -119,6 +119,7 @@ gboolean plugin_init(const char *enable, const char *disable)
const char *file;
char **cli_disabled, **cli_enabled;
unsigned int i;
+ int r;
/* Make a call to BtIO API so its symbols got resolved before the
* plugins are loaded. */
@@ -196,8 +197,14 @@ start:
for (list = plugins; list; list = list->next) {
struct bluetooth_plugin *plugin = list->data;
- if (plugin->desc->init() < 0) {
- error("Failed to init %s plugin", plugin->desc->name);
+ r = plugin->desc->init();
+ if (r < 0) {
+ if (r == -ENOSYS)
+ info("System does not support %s plugin",
+ plugin->desc->name);
+ else
+ error("Failed to init %s plugin",
+ plugin->desc->name);
continue;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bluez] bnep: don't error() if kernel lacks bnep module
2013-09-06 14:23 [PATCH bluez] bnep: don't error() if kernel lacks bnep module David Herrmann
@ 2013-09-13 10:01 ` Johan Hedberg
2013-09-13 10:38 ` David Herrmann
0 siblings, 1 reply; 5+ messages in thread
From: Johan Hedberg @ 2013-09-13 10:01 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, Marcel Holtmann
Hi David,
On Fri, Sep 06, 2013, David Herrmann wrote:
> If a user disables bnep kernel support, they normally do that on purpose.
> It's misleading to print an error during module-startup in this situation.
> Therefore, only print a hint that bnep-support is missing if
> EPROTONOSUPPORT is returned by the kernel.
>
> Furthermore, allow modules to forward ENOSYS as error to bluetoothd core
> to handle it as "module is not compatible with system setup" instead of an
> error. ENOSYS is commonly used to signal "missing kernel infrastructure"
> so it seems appropriate here.
> ---
> profiles/network/common.c | 7 ++++++-
> profiles/network/manager.c | 17 ++++++++++++++---
> src/plugin.c | 11 +++++++++--
> 3 files changed, 29 insertions(+), 6 deletions(-)
First of all, sorry for the delay with reviewing this. I forgot about it
and only bumped into it now when checking what I'd missed from the last
month in my linux-bluetooth folder.
> diff --git a/profiles/network/common.c b/profiles/network/common.c
> index e069892..17bff30 100644
> --- a/profiles/network/common.c
> +++ b/profiles/network/common.c
> @@ -110,8 +110,13 @@ int bnep_init(void)
>
> if (ctl < 0) {
> int err = -errno;
> - error("Failed to open control socket: %s (%d)",
> +
> + if (err == -EPROTONOSUPPORT)
> + info("kernel lacks bnep-protocol support");
I think warn() might be more appropriate here.
> diff --git a/profiles/network/manager.c b/profiles/network/manager.c
> index 03b1b3d..7697766 100644
> --- a/profiles/network/manager.c
> +++ b/profiles/network/manager.c
> @@ -25,6 +25,7 @@
> #include <config.h>
> #endif
>
> +#include <errno.h>
> #include <stdbool.h>
>
> #include <bluetooth/bluetooth.h>
> @@ -169,11 +170,21 @@ static struct btd_profile nap_profile = {
>
> static int network_init(void)
> {
> + int r;
Please use "err". That's the convention for integer error variables.
> + r = bnep_init();
> + if (r) {
> + if (r == -EPROTONOSUPPORT) {
> + info("bnep module not available, disabling plugin");
> + r = -ENOSYS;
> + } else {
> + error("Can't init bnep module");
> + r = -1;
I think it'd make sense to just pass forward whatever error bnep_init()
returned instead of mapping to -1 (which isn't even a valid error code -
we should fix this in several places of the existing code base).
> diff --git a/src/plugin.c b/src/plugin.c
> index 51c98bc..396c8af 100644
> --- a/src/plugin.c
> +++ b/src/plugin.c
> @@ -119,6 +119,7 @@ gboolean plugin_init(const char *enable, const char *disable)
> const char *file;
> char **cli_disabled, **cli_enabled;
> unsigned int i;
> + int r;
Again, please use "err".
> /* Make a call to BtIO API so its symbols got resolved before the
> * plugins are loaded. */
> @@ -196,8 +197,14 @@ start:
> for (list = plugins; list; list = list->next) {
> struct bluetooth_plugin *plugin = list->data;
>
> - if (plugin->desc->init() < 0) {
> - error("Failed to init %s plugin", plugin->desc->name);
> + r = plugin->desc->init();
> + if (r < 0) {
> + if (r == -ENOSYS)
> + info("System does not support %s plugin",
> + plugin->desc->name);
warn() might be more appropriate here.
> + else
> + error("Failed to init %s plugin",
> + plugin->desc->name);
> continue;
> }
In general this plugin.c update should probably go into its own patch
that precedes the network plugin patch.
Johan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bluez] bnep: don't error() if kernel lacks bnep module
2013-09-13 10:01 ` Johan Hedberg
@ 2013-09-13 10:38 ` David Herrmann
2013-09-13 10:58 ` Johan Hedberg
0 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2013-09-13 10:38 UTC (permalink / raw)
To: David Herrmann, linux-bluetooth@vger.kernel.org, Marcel Holtmann,
Johan Hedberg
Hi
On Fri, Sep 13, 2013 at 12:01 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi David,
>
> On Fri, Sep 06, 2013, David Herrmann wrote:
>> If a user disables bnep kernel support, they normally do that on purpose.
>> It's misleading to print an error during module-startup in this situation.
>> Therefore, only print a hint that bnep-support is missing if
>> EPROTONOSUPPORT is returned by the kernel.
>>
>> Furthermore, allow modules to forward ENOSYS as error to bluetoothd core
>> to handle it as "module is not compatible with system setup" instead of an
>> error. ENOSYS is commonly used to signal "missing kernel infrastructure"
>> so it seems appropriate here.
>> ---
>> profiles/network/common.c | 7 ++++++-
>> profiles/network/manager.c | 17 ++++++++++++++---
>> src/plugin.c | 11 +++++++++--
>> 3 files changed, 29 insertions(+), 6 deletions(-)
>
> First of all, sorry for the delay with reviewing this. I forgot about it
> and only bumped into it now when checking what I'd missed from the last
> month in my linux-bluetooth folder.
>
>> diff --git a/profiles/network/common.c b/profiles/network/common.c
>> index e069892..17bff30 100644
>> --- a/profiles/network/common.c
>> +++ b/profiles/network/common.c
>> @@ -110,8 +110,13 @@ int bnep_init(void)
>>
>> if (ctl < 0) {
>> int err = -errno;
>> - error("Failed to open control socket: %s (%d)",
>> +
>> + if (err == -EPROTONOSUPPORT)
>> + info("kernel lacks bnep-protocol support");
>
> I think warn() might be more appropriate here.
Using warn() defeats the whole purpose of this patch. I want to run
bluetoothd without getting a warning or error, and obviously, there is
nothing to warn _me_ about. I'm ok if you want err() or warn() here so
other users get warned, but in that case we should just drop the
patch.
So it's up to you to decide. If info() is ok, I will fix the issues
you mentioned below, otherwise just drop it.
Cheers
David
>> diff --git a/profiles/network/manager.c b/profiles/network/manager.c
>> index 03b1b3d..7697766 100644
>> --- a/profiles/network/manager.c
>> +++ b/profiles/network/manager.c
>> @@ -25,6 +25,7 @@
>> #include <config.h>
>> #endif
>>
>> +#include <errno.h>
>> #include <stdbool.h>
>>
>> #include <bluetooth/bluetooth.h>
>> @@ -169,11 +170,21 @@ static struct btd_profile nap_profile = {
>>
>> static int network_init(void)
>> {
>> + int r;
>
> Please use "err". That's the convention for integer error variables.
>
>> + r = bnep_init();
>> + if (r) {
>> + if (r == -EPROTONOSUPPORT) {
>> + info("bnep module not available, disabling plugin");
>> + r = -ENOSYS;
>> + } else {
>> + error("Can't init bnep module");
>> + r = -1;
>
> I think it'd make sense to just pass forward whatever error bnep_init()
> returned instead of mapping to -1 (which isn't even a valid error code -
> we should fix this in several places of the existing code base).
>
>> diff --git a/src/plugin.c b/src/plugin.c
>> index 51c98bc..396c8af 100644
>> --- a/src/plugin.c
>> +++ b/src/plugin.c
>> @@ -119,6 +119,7 @@ gboolean plugin_init(const char *enable, const char *disable)
>> const char *file;
>> char **cli_disabled, **cli_enabled;
>> unsigned int i;
>> + int r;
>
> Again, please use "err".
>
>> /* Make a call to BtIO API so its symbols got resolved before the
>> * plugins are loaded. */
>> @@ -196,8 +197,14 @@ start:
>> for (list = plugins; list; list = list->next) {
>> struct bluetooth_plugin *plugin = list->data;
>>
>> - if (plugin->desc->init() < 0) {
>> - error("Failed to init %s plugin", plugin->desc->name);
>> + r = plugin->desc->init();
>> + if (r < 0) {
>> + if (r == -ENOSYS)
>> + info("System does not support %s plugin",
>> + plugin->desc->name);
>
> warn() might be more appropriate here.
>
>> + else
>> + error("Failed to init %s plugin",
>> + plugin->desc->name);
>> continue;
>> }
>
> In general this plugin.c update should probably go into its own patch
> that precedes the network plugin patch.
>
> Johan
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bluez] bnep: don't error() if kernel lacks bnep module
2013-09-13 10:38 ` David Herrmann
@ 2013-09-13 10:58 ` Johan Hedberg
2013-09-13 11:07 ` David Herrmann
0 siblings, 1 reply; 5+ messages in thread
From: Johan Hedberg @ 2013-09-13 10:58 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth@vger.kernel.org, Marcel Holtmann
Hi David,
On Fri, Sep 13, 2013, David Herrmann wrote:
> On Fri, Sep 13, 2013 at 12:01 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi David,
> >
> > On Fri, Sep 06, 2013, David Herrmann wrote:
> >> If a user disables bnep kernel support, they normally do that on purpose.
> >> It's misleading to print an error during module-startup in this situation.
> >> Therefore, only print a hint that bnep-support is missing if
> >> EPROTONOSUPPORT is returned by the kernel.
> >>
> >> Furthermore, allow modules to forward ENOSYS as error to bluetoothd core
> >> to handle it as "module is not compatible with system setup" instead of an
> >> error. ENOSYS is commonly used to signal "missing kernel infrastructure"
> >> so it seems appropriate here.
> >> ---
> >> profiles/network/common.c | 7 ++++++-
> >> profiles/network/manager.c | 17 ++++++++++++++---
> >> src/plugin.c | 11 +++++++++--
> >> 3 files changed, 29 insertions(+), 6 deletions(-)
> >
> > First of all, sorry for the delay with reviewing this. I forgot about it
> > and only bumped into it now when checking what I'd missed from the last
> > month in my linux-bluetooth folder.
> >
> >> diff --git a/profiles/network/common.c b/profiles/network/common.c
> >> index e069892..17bff30 100644
> >> --- a/profiles/network/common.c
> >> +++ b/profiles/network/common.c
> >> @@ -110,8 +110,13 @@ int bnep_init(void)
> >>
> >> if (ctl < 0) {
> >> int err = -errno;
> >> - error("Failed to open control socket: %s (%d)",
> >> +
> >> + if (err == -EPROTONOSUPPORT)
> >> + info("kernel lacks bnep-protocol support");
> >
> > I think warn() might be more appropriate here.
>
> Using warn() defeats the whole purpose of this patch. I want to run
> bluetoothd without getting a warning or error, and obviously, there is
> nothing to warn _me_ about. I'm ok if you want err() or warn() here so
> other users get warned, but in that case we should just drop the
> patch.
I thought the main idea was to have a clearer log message when the
module is not there. None of the warn/error/info messages can be
suppressed (unlike DBG which is off by default). To me warn() seems more
appropriate than info() since it's meaning is "this is something that
may or may not be of concern to the user".
> So it's up to you to decide. If info() is ok, I will fix the issues
> you mentioned below, otherwise just drop it.
I do think the patch has value even though info() is not used by having
an understandable log message for what's going on.
Johan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bluez] bnep: don't error() if kernel lacks bnep module
2013-09-13 10:58 ` Johan Hedberg
@ 2013-09-13 11:07 ` David Herrmann
0 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2013-09-13 11:07 UTC (permalink / raw)
To: David Herrmann, linux-bluetooth@vger.kernel.org, Marcel Holtmann
Hi
On Fri, Sep 13, 2013 at 12:58 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi David,
>
> On Fri, Sep 13, 2013, David Herrmann wrote:
>> On Fri, Sep 13, 2013 at 12:01 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> > Hi David,
>> >
>> > On Fri, Sep 06, 2013, David Herrmann wrote:
>> >> If a user disables bnep kernel support, they normally do that on purpose.
>> >> It's misleading to print an error during module-startup in this situation.
>> >> Therefore, only print a hint that bnep-support is missing if
>> >> EPROTONOSUPPORT is returned by the kernel.
>> >>
>> >> Furthermore, allow modules to forward ENOSYS as error to bluetoothd core
>> >> to handle it as "module is not compatible with system setup" instead of an
>> >> error. ENOSYS is commonly used to signal "missing kernel infrastructure"
>> >> so it seems appropriate here.
>> >> ---
>> >> profiles/network/common.c | 7 ++++++-
>> >> profiles/network/manager.c | 17 ++++++++++++++---
>> >> src/plugin.c | 11 +++++++++--
>> >> 3 files changed, 29 insertions(+), 6 deletions(-)
>> >
>> > First of all, sorry for the delay with reviewing this. I forgot about it
>> > and only bumped into it now when checking what I'd missed from the last
>> > month in my linux-bluetooth folder.
>> >
>> >> diff --git a/profiles/network/common.c b/profiles/network/common.c
>> >> index e069892..17bff30 100644
>> >> --- a/profiles/network/common.c
>> >> +++ b/profiles/network/common.c
>> >> @@ -110,8 +110,13 @@ int bnep_init(void)
>> >>
>> >> if (ctl < 0) {
>> >> int err = -errno;
>> >> - error("Failed to open control socket: %s (%d)",
>> >> +
>> >> + if (err == -EPROTONOSUPPORT)
>> >> + info("kernel lacks bnep-protocol support");
>> >
>> > I think warn() might be more appropriate here.
>>
>> Using warn() defeats the whole purpose of this patch. I want to run
>> bluetoothd without getting a warning or error, and obviously, there is
>> nothing to warn _me_ about. I'm ok if you want err() or warn() here so
>> other users get warned, but in that case we should just drop the
>> patch.
>
> I thought the main idea was to have a clearer log message when the
> module is not there. None of the warn/error/info messages can be
> suppressed (unlike DBG which is off by default). To me warn() seems more
> appropriate than info() since it's meaning is "this is something that
> may or may not be of concern to the user".
It's not about suppressing messages. It's about putting proper
attributes on them. If I look at my system-log, I am usually only
interested in errors and warnings. And each of them needs my attention
to get fixed. Otherwise, the purpose of an error or warning is missed.
If every running daemon spits warnings to the log which I cannot
"fix", warnings will no longer be any special. Same for errors,
obviously.
So imho every warning should be "fixable" by the administrator. And in
this case I don't think enabling BNEP in the kernel is the appropriate
fix. Instead, if an administrator disables BNEP, bluetoothd should
interpret that as a "configuration-choice".
>> So it's up to you to decide. If info() is ok, I will fix the issues
>> you mentioned below, otherwise just drop it.
>
> I do think the patch has value even though info() is not used by having
> an understandable log message for what's going on.
In that case I will resend it. Please let me know whether to use
info() or warn().
Thanks
David
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-13 11:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 14:23 [PATCH bluez] bnep: don't error() if kernel lacks bnep module David Herrmann
2013-09-13 10:01 ` Johan Hedberg
2013-09-13 10:38 ` David Herrmann
2013-09-13 10:58 ` Johan Hedberg
2013-09-13 11:07 ` David Herrmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).