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?
> ---
> 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
