Re: [PATCH] device: Don’t report EALREADY if enabling device succeeds with ifup

[Philip Withnall]

On Mon, 2015-03-16 at 12:58 +0200, Patrik Flykt wrote:
> On Fri, 2015-03-13 at 08:31 +0000, 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

[Philip Withnall]

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

[Philip Withnall]

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 @@
 Policy prevents modification of settings
 
   no
-  auth_self_keep_session
+  auth_self_keep
 
   
 
@@ -22,7 +22,7 @@
 Policy prevents modification of secrets
 
   no
-  auth_admin_keep_session
+  auth_admin_keep
 
   
 
-- 
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