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
[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
[PATCH] plugins: Fix polkit policy defaults to use current allow values
The *_session variants of the policy default allow values were removed in 2007: http://lists.freedesktop.org/archives/hal-commit/2007-August/003690.html Instead of auth_self_keep_session, use auth_self_keep. Similarly for auth_admin_keep_session. This fixes parsing the default policy with any recent version of polkit — tested with version 0.105. Previously, I believe polkit aborted parsing the policy and hence never added the actions to its action pool for use in rule files or authorisations. --- plugins/polkit.policy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/polkit.policy b/plugins/polkit.policy index 0c2629a..0de152c 100644 --- a/plugins/polkit.policy +++ b/plugins/polkit.policy @@ -13,7 +13,7 @@ messagePolicy prevents modification of settings/message defaults allow_inactiveno/allow_inactive - allow_activeauth_self_keep_session/allow_active + allow_activeauth_self_keep/allow_active /defaults /action @@ -22,7 +22,7 @@ messagePolicy prevents modification of secrets/message defaults allow_inactiveno/allow_inactive - allow_activeauth_admin_keep_session/allow_active + allow_activeauth_admin_keep/allow_active /defaults /action -- 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
