Re: [PATCH] device: Don’t report EALREADY if enabling device succeeds with ifup
On Wed, 2015-03-18 at 17:06 +, Philip Withnall wrote: Whew, right. I am unlikely to find time to work on this until one or three weeks from now, but I have put it on my backlog to look at as soon as I can after then. Sorry! No problem. Thanks for taking time to report the issue and solving it at least in one way - it works that way too actually. The only issue I have is that there is a bigger risk of someone forgetting what errors devices should or should not return, thus the proposed more generic task of figuring out what response is appropriate from the technology API. Doing it quickly makes me believe that -EALREADY should be treated as success (= 0), but the details matter. I'll look into this a bit. Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] device: Don’t report EALREADY if enabling device succeeds with ifup
On Mon, 2015-03-16 at 12:58 +0200, Patrik Flykt wrote:
On Fri, 2015-03-13 at 08:31 +, Philip Withnall wrote:
$ connmanctl disable ethernet
Disabled ethernet
$ connmanctl technologies
/net/connman/technology/ethernet
Powered = False
…
$ connmanctl enable ethernet
Error ethernet: Already enabled
Did anything special happen between checking that the Powered status was
False and enabling it again? Did 'monitor technologies' show Powered
changing state in between?
Nope, nothing changed in between.
---
src/device.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/device.c b/src/device.c
index c0683ab..c9e9b8b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -168,7 +168,7 @@ static gboolean device_pending_reset(gpointer user_data)
int __connman_device_enable(struct connman_device *device)
{
- int err;
+ int err, index_err = -EOPNOTSUPP;
DBG(device %p, device);
@@ -186,9 +186,9 @@ int __connman_device_enable(struct connman_device
*device)
return -EALREADY;
if (device-index 0) {
- err = connman_inet_ifup(device-index);
- if (err 0 err != -EALREADY)
- return err;
+ index_err = connman_inet_ifup(device-index);
+ if (index_err 0 index_err != -EALREADY)
+ return index_err;
}
device-powered_pending = PENDING_ENABLE;
@@ -197,9 +197,14 @@ int __connman_device_enable(struct connman_device
*device)
/*
* device gets enabled right away.
* Invoke the callback
+*
+* If device-driver-enable() returned EALREADY but the earlier
+* connman_inet_ifup() call did not, then the interface came up
+* with the earlier call and we should not report an error.
*/
- if (err == 0) {
+ if (err == 0 || (err == -EALREADY index_err == 0)) {
connman_device_set_powered(device, true);
+ err = 0;
goto done;
}
This check is a bit too low in the stack. In device.c it is ok to report
-EALREADY for situations that are already in the state they're attempted
to be set to.
There are actually two bugs in technology.c.
The first bug is in technology_enable(), once past the statement 'if
(technology-enabled)...' there is no reason to reply with -EALREADY.
For some reason ConnMan's powered property doesn't reflect the reality
correctly, but it does not really matter as being a cause for -EALREADY.
So the first fix is to set err to zero in case of an -EALREADY from
either __connman_rfkill_block() or technology_affect_devices() before
returning.
The second bug is in technology_affect_devices(). Only when all affected
devices report an error should technology_affect_devices() return an
error, not the result from the device that happens to be the last one in
the list and fail as is happening now.
An then an additional bug is in technology_disable(), it does the same
thing wrong as technology_enable()...
Whew, right. I am unlikely to find time to work on this until one or
three weeks from now, but I have put it on my backlog to look at as soon
as I can after then. Sorry!
Philip
signature.asc
Description: This is a digitally signed message part
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] device: Don’t report EALREADY if enabling device succeeds with ifup
On Fri, 2015-03-13 at 08:31 +, Philip Withnall wrote:
$ connmanctl disable ethernet
Disabled ethernet
$ connmanctl technologies
/net/connman/technology/ethernet
Powered = False
…
$ connmanctl enable ethernet
Error ethernet: Already enabled
Did anything special happen between checking that the Powered status was
False and enabling it again? Did 'monitor technologies' show Powered
changing state in between?
---
src/device.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/device.c b/src/device.c
index c0683ab..c9e9b8b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -168,7 +168,7 @@ static gboolean device_pending_reset(gpointer user_data)
int __connman_device_enable(struct connman_device *device)
{
- int err;
+ int err, index_err = -EOPNOTSUPP;
DBG(device %p, device);
@@ -186,9 +186,9 @@ int __connman_device_enable(struct connman_device *device)
return -EALREADY;
if (device-index 0) {
- err = connman_inet_ifup(device-index);
- if (err 0 err != -EALREADY)
- return err;
+ index_err = connman_inet_ifup(device-index);
+ if (index_err 0 index_err != -EALREADY)
+ return index_err;
}
device-powered_pending = PENDING_ENABLE;
@@ -197,9 +197,14 @@ int __connman_device_enable(struct connman_device
*device)
/*
* device gets enabled right away.
* Invoke the callback
+ *
+ * If device-driver-enable() returned EALREADY but the earlier
+ * connman_inet_ifup() call did not, then the interface came up
+ * with the earlier call and we should not report an error.
*/
- if (err == 0) {
+ if (err == 0 || (err == -EALREADY index_err == 0)) {
connman_device_set_powered(device, true);
+ err = 0;
goto done;
}
This check is a bit too low in the stack. In device.c it is ok to report
-EALREADY for situations that are already in the state they're attempted
to be set to.
There are actually two bugs in technology.c.
The first bug is in technology_enable(), once past the statement 'if
(technology-enabled)...' there is no reason to reply with -EALREADY.
For some reason ConnMan's powered property doesn't reflect the reality
correctly, but it does not really matter as being a cause for -EALREADY.
So the first fix is to set err to zero in case of an -EALREADY from
either __connman_rfkill_block() or technology_affect_devices() before
returning.
The second bug is in technology_affect_devices(). Only when all affected
devices report an error should technology_affect_devices() return an
error, not the result from the device that happens to be the last one in
the list and fail as is happening now.
An then an additional bug is in technology_disable(), it does the same
thing wrong as technology_enable()...
Good catch!
Cheers,
Patrik
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] device: Don’t report EALREADY if enabling device succeeds with ifup
If a device is indexed (for example, a wired ethernet connection), it
can be successfully enabled by calling connman_inet_ifup(). This means
that the call to device-driver-enable() lower down in
__connman_device_enable() will fail with EALREADY. Squash that error,
since the overall operation has been a success.
This can be reproduced on a system with a wired connection using
connmanctl:
$ connmanctl technologies
/net/connman/technology/ethernet
Name = Wired
Type = ethernet
Powered = True
Connected = True
Tethering = False
$ connmanctl disable ethernet
Disabled ethernet
$ connmanctl technologies
/net/connman/technology/ethernet
Powered = False
…
$ connmanctl enable ethernet
Error ethernet: Already enabled
With the patch applied, the final call succeeds as expected.
---
src/device.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/device.c b/src/device.c
index c0683ab..c9e9b8b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -168,7 +168,7 @@ static gboolean device_pending_reset(gpointer user_data)
int __connman_device_enable(struct connman_device *device)
{
- int err;
+ int err, index_err = -EOPNOTSUPP;
DBG(device %p, device);
@@ -186,9 +186,9 @@ int __connman_device_enable(struct connman_device *device)
return -EALREADY;
if (device-index 0) {
- err = connman_inet_ifup(device-index);
- if (err 0 err != -EALREADY)
- return err;
+ index_err = connman_inet_ifup(device-index);
+ if (index_err 0 index_err != -EALREADY)
+ return index_err;
}
device-powered_pending = PENDING_ENABLE;
@@ -197,9 +197,14 @@ int __connman_device_enable(struct connman_device *device)
/*
* device gets enabled right away.
* Invoke the callback
+*
+* If device-driver-enable() returned EALREADY but the earlier
+* connman_inet_ifup() call did not, then the interface came up
+* with the earlier call and we should not report an error.
*/
- if (err == 0) {
+ if (err == 0 || (err == -EALREADY index_err == 0)) {
connman_device_set_powered(device, true);
+ err = 0;
goto done;
}
--
1.9.3
signature.asc
Description: This is a digitally signed message part
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
