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 [email protected] https://lists.connman.net/mailman/listinfo/connman
