Send connman mailing list submissions to
        connman@lists.01.org

To subscribe or unsubscribe via the World Wide Web, visit
        https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
        connman-requ...@lists.01.org

You can reach the person managing the list at
        connman-ow...@lists.01.org

When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."


Today's Topics:

   1. Re: [PATCH 8/8] resolver: MAXDNSSRCH definition for android
      (Patrik Flykt)
   2. [PATCH] tethering: Add verification to bridge creation and
      configuration (Jose Blanquicet)
   3. Re: A couple of question regarding tethering (Jose Blanquicet)


----------------------------------------------------------------------

Message: 1
Date: Thu, 22 Sep 2016 09:21:34 +0300
From: Patrik Flykt <patrik.fl...@linux.intel.com>
To: Naveen Singh <nasi...@google.com>, connman@lists.01.org
Subject: Re: [PATCH 8/8] resolver: MAXDNSSRCH definition for android
Message-ID: <1474525294.11736.66.ca...@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"

On Wed, 2016-09-21 at 15:59 -0700, Naveen Singh wrote:
> MAXDNSSRCH is defined in resolv_params.h in bionic. This commit
> includes
> this file for android compilation.
> ---
> ?src/resolver.c | 3 +++
> ?1 file changed, 3 insertions(+)
> 
> diff --git a/src/resolver.c b/src/resolver.c
> index fbe4be7..3059353 100644
> --- a/src/resolver.c
> +++ b/src/resolver.c
> @@ -32,6 +32,9 @@
> ?#include <sys/stat.h>
> ?#include <resolv.h>
> ?#include <netdb.h>
> +#ifdef ANDROID_COMPILE
> +#include <resolv_params.h>
> +#endif

AC_CHECK_FILE() for resolv_params.h (or whatever it actually was
called) works out nicely here?

        Patrik

> ?#include "connman.h"
> ?


------------------------------

Message: 2
Date: Thu, 22 Sep 2016 09:54:56 +0200
From: Jose Blanquicet <blanqui...@gmail.com>
To: connman@lists.01.org
Subject: [PATCH] tethering: Add verification to bridge creation and
        configuration
Message-ID: <1474530896-16726-1-git-send-email-blanqui...@gmail.com>

In case there is an error while setting up the bridge for WiFi tethering, 
ConnMan will anyway indicate success and the removed interface will remain
removed and could not be use anymore. This patch aims to avoid such a situation
by adding a verification after setting up the bridge set-up and in case there is
an error stop the process and do not remove the interface.

Integration of this verification to others tethering technologies has to be
evaluated and implemented in case it is advantageous.

---
 include/technology.h |  2 +-
 plugins/bluetooth.c  |  5 +++++
 plugins/ethernet.c   |  5 +++++
 plugins/gadget.c     |  5 +++++
 plugins/wifi.c       | 23 ++++++++++++++++++-----
 src/connman.h        |  2 +-
 src/technology.c     | 19 ++++++++++++-------
 src/tethering.c      | 16 +++++++++-------
 8 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/include/technology.h b/include/technology.h
index d7fcdde..97db660 100644
--- a/include/technology.h
+++ b/include/technology.h
@@ -36,7 +36,7 @@ extern "C" {
 
 struct connman_technology;
 
-void connman_technology_tethering_notify(struct connman_technology *technology,
+int connman_technology_tethering_notify(struct connman_technology *technology,
                                                        bool enabled);
 int connman_technology_set_regdom(const char *alpha2);
 void connman_technology_regdom_notify(struct connman_technology *technology,
diff --git a/plugins/bluetooth.c b/plugins/bluetooth.c
index 401bb30..46ac7e6 100644
--- a/plugins/bluetooth.c
+++ b/plugins/bluetooth.c
@@ -692,6 +692,11 @@ static void tethering_create_cb(DBusMessage *message, void 
*user_data)
        DBG("bridge %s %s", tethering->bridge, tethering->enable ?
                        "enabled": "disabled");
 
+       /*
+        * TODO: In case of enabling, use return value to check
+        * if bridge was created and DHCP Server was
+        * configured/started successfully before continuing.
+        */
        if (tethering->technology)
                connman_technology_tethering_notify(tethering->technology,
                                tethering->enable);
diff --git a/plugins/ethernet.c b/plugins/ethernet.c
index 9a4d741..942b003 100644
--- a/plugins/ethernet.c
+++ b/plugins/ethernet.c
@@ -371,6 +371,11 @@ static void eth_tech_enable_tethering(struct 
connman_technology *technology,
                                remove_network(device, ethernet);
                }
 
+               /*
+                * TODO: Use return value to check if bridge was created
+                * and DHCP Server was configured/started successfully
+                * before continuing.
+                */
                connman_technology_tethering_notify(technology, true);
 
                connman_inet_ifup(index);
diff --git a/plugins/gadget.c b/plugins/gadget.c
index 94f6648..fbe784f 100644
--- a/plugins/gadget.c
+++ b/plugins/gadget.c
@@ -265,6 +265,11 @@ static void gadget_tech_enable_tethering(struct 
connman_technology *technology,
                                remove_network(device, gadget);
                }
 
+               /*
+                * TODO: Use return value to check if bridge was created
+                * and DHCP Server was configured/started successfully
+                * before continuing.
+                */
                connman_technology_tethering_notify(technology, true);
 
                connman_inet_ifup(index);
diff --git a/plugins/wifi.c b/plugins/wifi.c
index 8bde9c0..1b25739 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -3138,13 +3138,14 @@ static void sta_remove_callback(int result,
        DBG("ifname %s result %d ", info->ifname, result);
 
        if ((result < 0) || (info->wifi->ap_supported != WIFI_AP_SUPPORTED)) {
-               info->wifi->tethering = true;
+               info->wifi->tethering = false;
+               connman_technology_tethering_notify(info->technology, false);
 
                g_free(info->ifname);
                g_free(info->ssid);
                g_free(info);
 
-               if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
+               if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
                        g_free(info->wifi->tethering_param->ssid);
                        g_free(info->wifi->tethering_param);
                        info->wifi->tethering_param = NULL;
@@ -3154,8 +3155,6 @@ static void sta_remove_callback(int result,
 
        info->wifi->interface = NULL;
 
-       connman_technology_tethering_notify(info->technology, true);
-
        g_supplicant_interface_create(info->ifname, driver, info->wifi->bridge,
                                                ap_create_callback,
                                                        info);
@@ -3171,7 +3170,7 @@ static int enable_wifi_tethering(struct 
connman_technology *technology,
        struct wifi_tethering_info *info;
        const char *ifname;
        unsigned int mode;
-       int err;
+       int err, bridge_result = 0;
 
        for (list = iface_list; list; list = list->next) {
                wifi = list->data;
@@ -3230,6 +3229,11 @@ static int enable_wifi_tethering(struct 
connman_technology *technology,
                info->wifi->tethering = true;
                info->wifi->ap_supported = WIFI_AP_SUPPORTED;
 
+               bridge_result =
+                       connman_technology_tethering_notify(technology, true);
+               if (bridge_result < 0)
+                       goto failed;
+
                err = g_supplicant_interface_remove(interface,
                                                sta_remove_callback,
                                                        info);
@@ -3244,6 +3248,15 @@ static int enable_wifi_tethering(struct 
connman_technology *technology,
                g_free(info);
                g_free(wifi->tethering_param);
                wifi->tethering_param = NULL;
+
+               if (bridge_result < 0) {
+                       /*
+                        * If it was an error while setting up the bridge then
+                        * do not try again on another interface, bridge set-up
+                        * does not depend on it.
+                        */
+                       break;
+               }
        }
 
        return -EOPNOTSUPP;
diff --git a/src/connman.h b/src/connman.h
index 401e3d7..ce3ef8d 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -620,7 +620,7 @@ int __connman_tethering_init(void);
 void __connman_tethering_cleanup(void);
 
 const char *__connman_tethering_get_bridge(void);
-void __connman_tethering_set_enabled(void);
+int __connman_tethering_set_enabled(void);
 void __connman_tethering_set_disabled(void);
 
 int __connman_private_network_request(DBusMessage *msg, const char *owner);
diff --git a/src/technology.c b/src/technology.c
index 660af52..6f91af7 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -211,22 +211,27 @@ static void tethering_changed(struct connman_technology 
*technology)
        technology_save(technology);
 }
 
-void connman_technology_tethering_notify(struct connman_technology *technology,
+int connman_technology_tethering_notify(struct connman_technology *technology,
                                                        bool enabled)
 {
+       int err;
+
        DBG("technology %p enabled %u", technology, enabled);
 
        if (technology->tethering == enabled)
-               return;
+               return -EALREADY;
 
-       technology->tethering = enabled;
+       if (enabled) {
+               err = __connman_tethering_set_enabled();
+               if (err < 0)
+                       return err;
+       } else
+               __connman_tethering_set_disabled();
 
+       technology->tethering = enabled;
        tethering_changed(technology);
 
-       if (enabled)
-               __connman_tethering_set_enabled();
-       else
-               __connman_tethering_set_disabled();
+       return 0;
 }
 
 static int set_tethering(struct connman_technology *technology,
diff --git a/src/tethering.c b/src/tethering.c
index 3153349..9d72bce 100644
--- a/src/tethering.c
+++ b/src/tethering.c
@@ -181,7 +181,7 @@ static void tethering_restart(struct connman_ippool *pool, 
void *user_data)
        __connman_tethering_set_enabled();
 }
 
-void __connman_tethering_set_enabled(void)
+int __connman_tethering_set_enabled(void)
 {
        int index;
        int err;
@@ -197,12 +197,12 @@ void __connman_tethering_set_enabled(void)
        DBG("enabled %d", tethering_enabled + 1);
 
        if (__sync_fetch_and_add(&tethering_enabled, 1) != 0)
-               return;
+               return -EACCES;
 
        err = __connman_bridge_create(BRIDGE_NAME);
        if (err < 0) {
                __sync_fetch_and_sub(&tethering_enabled, 1);
-               return;
+               return -EOPNOTSUPP;
        }
 
        index = connman_inet_ifindex(BRIDGE_NAME);
@@ -212,7 +212,7 @@ void __connman_tethering_set_enabled(void)
                connman_error("Fail to create IP pool");
                __connman_bridge_remove(BRIDGE_NAME);
                __sync_fetch_and_sub(&tethering_enabled, 1);
-               return;
+               return -EADDRNOTAVAIL;
        }
 
        gateway = __connman_ippool_get_gateway(dhcp_ippool);
@@ -228,7 +228,7 @@ void __connman_tethering_set_enabled(void)
                __connman_ippool_unref(dhcp_ippool);
                __connman_bridge_remove(BRIDGE_NAME);
                __sync_fetch_and_sub(&tethering_enabled, 1);
-               return;
+               return -EADDRNOTAVAIL;
        }
 
        ns = connman_setting_get_string_list("FallbackNameservers");
@@ -264,7 +264,7 @@ void __connman_tethering_set_enabled(void)
                __connman_ippool_unref(dhcp_ippool);
                __connman_bridge_remove(BRIDGE_NAME);
                __sync_fetch_and_sub(&tethering_enabled, 1);
-               return;
+               return -EOPNOTSUPP;
        }
 
        prefixlen = connman_ipaddress_calc_netmask_len(subnet_mask);
@@ -276,7 +276,7 @@ void __connman_tethering_set_enabled(void)
                __connman_ippool_unref(dhcp_ippool);
                __connman_bridge_remove(BRIDGE_NAME);
                __sync_fetch_and_sub(&tethering_enabled, 1);
-               return;
+               return -EOPNOTSUPP;
        }
 
        err = __connman_ipv6pd_setup(BRIDGE_NAME);
@@ -285,6 +285,8 @@ void __connman_tethering_set_enabled(void)
                        strerror(-err));
 
        DBG("tethering started");
+
+       return 0;
 }
 
 void __connman_tethering_set_disabled(void)
-- 
1.9.1



------------------------------

Message: 3
Date: Thu, 22 Sep 2016 09:57:35 +0200
From: Jose Blanquicet <blanqui...@gmail.com>
To: Patrik Flykt <patrik.fl...@linux.intel.com>
Cc: connman@lists.01.org,  "MANIEZZO Marco (MM)"
        <marco.manie...@magnetimarelli.com>
Subject: Re: A couple of question regarding tethering
Message-ID:
        <cafc8ijk18nbvcywsmtfsgrzxshp1pjy-crqvmb8hctrzesp...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8

Hi Patrik,

thanks for your detailed explanation. It was really useful!

>> 1. If I am not wrong, the return value when setting up an AP in
>> technology.c:set_tethering() can only be either 0, -EINPROGRESS or
>> -EOPNOTSUPP. Why it was implemented in such a way? This
>> implementation
>> prevents to forward any occurred error in driver->set_tethering()
>> function.
>
> As set_tethering() may have more than one driver to try, it will try to
> get a decent answer to the caller. It will return 0 if no driver
> returned error, -EINPROGRESS if at least one driver is still enabling
> tethering or -EOPNOTSUPP if no driver supported tethering. Therefore it
> is not useful to report the status of an individual driver.

It makes sense for me.

>> 2. There is a specific reason to continue trying to call
>> set_tethering() on others drivers after one of them has already
>> replied 0 or -EINPROGRESS? Why do not simply break the loop and
>> return?
>
> This comes back to the fact that tethering is done per technology,
> which means all devices for that technology are supposed to be used for
> tethering. This is to cover for the situation that there are e.g. two
> ethernet cards on the device, and tethering is enabled for ethernet
> technology as a whole. It is not possible to specify a particular card,
> as the individual cards are not exposed to the user.

Ok. I think I got the motivation of this.

>> 3. Are we sure tethering.c:__connman_tethering_set_enabled() will
>> always complete successfully? I would prefer to add a return value to
>> this function and also to connman_technology_tethering_notify() in
>> order to check that all the operations done inside
>> __connman_tethering_set_enabled (Create the bridge and
>> start/configure
>> DHCP server) have finished successfully before indicating tethering
>> was enabled and recreating the interface (Bridged with "tether") in
>> wifi.c:sta_remove_callback(). In fact, I would suggest to try to call
>> connman_technology_tethering_notify() before removing the wifi
>> interface. Does it make sense for you?
>> As you could see I mostly referred to wifi technology (plugin),
>> however this new return value could also be used by other
>> technologies
>> supporting tethering (bluetooth, gadget, ethernet).
>
> Notice that tethering can be enabled for all these technologies, but
> enabling the bridge i/f and dhcp server needs to happen only once. Yes,
> error values should perhaps be propagated and the error cases were (and
> are) not really accounted for in the calling code. So yes, errors
> should be communicated, but then again issues have not really been a
> problem in real life, at least I can't remember any being reported
> lately.

When I said "call connman_technology_tethering_notify() before
removing the wifi inteface" I meant still inside
enable_wifi_tethering() which still ensure it happens only once
(Please see PATCH). Although neither we have gotten problems so far,
this is a modification we would like to implement and share with the
community. Mostly because in case of errors, I saw that there is not a
"recovery plan" for this, the removed wifi interface will remain
removed and it could not be used anymore. I am sending a PATCH that
aims to check if there is any error while creating/configuring the
bridge and in particular for wifi cases, move this procedure before
removing the interface to avoid it remains removed in case of errors.
This return value could be also used for the other tethering
technologies but I am not an expert on those others so I prefer
someone else does this rather than me, I just added the comment.
Please, have a look at the PATCH.

Regards,

Jose Blanquicet


------------------------------

Subject: Digest Footer

_______________________________________________
connman mailing list
connman@lists.01.org
https://lists.01.org/mailman/listinfo/connman


------------------------------

End of connman Digest, Vol 11, Issue 26
***************************************

Reply via email to