Hi,
On Thu, 2015-04-09 at 16:19 +0530, Saurav Babu wrote:
> This patch properly removes nameservers from resolv.conf before clearing
> nameservers in service structure when IPv4.Configurations and
> Nameservers.Configurations are changed simultaneously.
>
> Connman was being run with nodnsproxy flag enabled.
> Checked using scripts of connman's test directory:
> set-ipv4-method <profile> manual <ip> <subnet> <gw>; set-nameservers
> <profile> <dns> <dns>
> set-ipv4-method <profile> dhcp; set-nameservers <profile>
>
> There are two possible cases:
> CASE 1: Nameservers and IP Configurations are changed from manual to DHCP
> In this case nameservers in service structure was cleared without
> removing from resolv.conf in function __connman_service_nameserver_remove()
> if there was only one nameservers. When there were more than one nameservers
> then nameservers structure was updated with the nameserver passed in
> argument of __connman_service_nameserver_remove() so other nameservers
> were not getting removed. This patch updates nameservers structure with
> all the other nameservers which are not passed to this function and the
> one passed to this function is removed from both service's nameservers
> structure and resolv.conf.
>
> CASE 2: Nameservers and IP Configurations are changed from DHCP to manual
> In this case there were few scenarios where connman's service was in
> Configuration state while trying to remove nameservers_config from
> service, and nameservers_config were not removed from resolv.conf. This
> patch removes the nameservers_config with the index of service rather
> than checking the service's connection state.
> ---
> src/service.c | 36 ++++++++++++++++++++++--------------
> 1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/src/service.c b/src/service.c
> index 7538bdd..c36f9e3 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -1129,9 +1129,9 @@ int __connman_service_nameserver_append(struct
> connman_service *service,
> int __connman_service_nameserver_remove(struct connman_service *service,
> const char *nameserver, bool is_auto)
> {
> - char **servers, **nameservers;
> + char **servers, **nameservers, **servers_remove;
> bool found = false;
> - int len, i, j;
> + int len, i, j, k, index;
>
> DBG("service %p nameserver %s auto %d", service, nameserver, is_auto);
>
> @@ -1157,30 +1157,37 @@ int __connman_service_nameserver_remove(struct
> connman_service *service,
>
> len = g_strv_length(nameservers);
>
> - if (len == 1) {
> - g_strfreev(nameservers);
> - if (is_auto)
> - service->nameservers_auto = NULL;
> - else
> - service->nameservers = NULL;
> -
> - return 0;
> - }
> -
The difference is that if all nameservers are removed,
service->nameservers* will now always point to an array of char *, where
the first and only member is NULL. I suppose you have ensured that the
remaining code in service.c still also works with a zero-length
nameserver list?
Maybe it's better to make a goto to the line before 'index = ...'
instead of removing this part?
> servers = g_try_new0(char *, len);
> if (!servers)
> return -ENOMEM;
>
> - for (i = 0, j = 0; i < len; i++) {
> + servers_remove = g_try_new0(char *, len);
> + if (!servers_remove)
> + return -ENOMEM;
> +
> + for (i = 0, j = 0, k = 0; i < len; i++) {
> if (g_strcmp0(nameservers[i], nameserver) != 0) {
> servers[j] = g_strdup(nameservers[i]);
The existing code is suboptimal. Just move the pointer to the servers[]
array instead of duplicating memory for it. Then...
> if (!servers[j])
> return -ENOMEM;
With the existing code memory is leaked should this fail.
> j++;
> + } else {
...remember to free the char * here in the else block and
unconditionally set nameservers[i] to NULL outside this if block so that
it won't be free'd afterwards.
> + servers_remove[k] = g_strdup(nameservers[i]);
Hmm, this is suboptimal. We are given one char * string as input, why
don't we just create a 'char *servers_remove[2] = { nameserver, NULL }'
in the beginning of this function? With that we don't need a 'k' in here
or assign anything at this point either.
> + if (!servers_remove[k])
> + return -ENOMEM;
With this we'd leak memory should anything go wrong here.
> + k++;
> }
> }
> servers[len - 1] = NULL;
> + servers_remove[k] = NULL;
> +
> + index = __connman_service_get_index(service);
> + if (index < 0)
> + return 0;
Please check that the index exists before allocating any memory. As it
is now, memory is leaked should this check fail.
Since the interface index is now used, please ensure that correct things
happen when two services, for example WiFi ones, are switched. I.e. one
service is taken down while the other service is being connected. In
some state(s) they will have the same interface index assigned, so one
has to figure out what the service state also is.
>
> + remove_nameservers(service, index, servers_remove);
> +
> + g_strfreev(servers_remove);
> g_strfreev(nameservers);
> nameservers = servers;
>
> @@ -3217,6 +3224,7 @@ static DBusMessage *set_property(DBusConnection *conn,
> GString *str;
> int index;
> const char *gw;
> + DBG("%s", name);
>
> if (__connman_provider_is_immutable(service->provider) ||
> service->immutable)
> @@ -3251,7 +3259,7 @@ static DBusMessage *set_property(DBusConnection *conn,
> }
> }
>
> - remove_nameservers(service, -1, service->nameservers_config);
> + remove_nameservers(service, index, service->nameservers_config);
> g_strfreev(service->nameservers_config);
>
> if (str->len > 0) {
Even if short, the modification to set_property is a fix for a different
issue. Can you send that as a separate patch?
Sorry it took so long to reply, been quite busy with other stuff lately.
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
