Hi Jukka,
My comments below:
On Mon, Nov 22, 2010 at 06:11:32PM +0200, Jukka Rissanen wrote:
> diff --git a/src/ipconfig.c b/src/ipconfig.c
> index a872dff..2e4a910 100644
> --- a/src/ipconfig.c
> +++ b/src/ipconfig.c
> @@ -50,8 +50,6 @@ struct connman_ipconfig {
> enum connman_ipconfig_method method;
> struct connman_ipaddress *address;
> struct connman_ipaddress *system;
> -
> - struct connman_ipconfig *ipv6;
> };
>
> struct connman_ipdevice {
> @@ -76,10 +74,14 @@ struct connman_ipdevice {
>
> char *pac;
>
> - struct connman_ipconfig *config;
> + struct connman_ipconfig *config_ipv4;
> + struct connman_ipconfig *config_ipv6;
> +
> + struct connman_ipconfig_driver *driver_ipv4;
> + struct connman_ipconfig *driver_config_ipv4;
>
> - struct connman_ipconfig_driver *driver;
> - struct connman_ipconfig *driver_config;
> + struct connman_ipconfig_driver *driver_ipv6;
> + struct connman_ipconfig *driver_config_ipv6;
So the whole connman_ipconfig_driver stuff is not used anywhere, and I'm
planning to remove it sooner than later. So the chunks here and below would
probably become irrelevant.
You should not care about it for now, just a heads up.
> @@ -431,7 +501,7 @@ static void update_stats(struct connman_ipdevice
> *ipdevice,
> connman_info("%s {TX} %u packets %u bytes", ipdevice->ifname,
> stats->tx_packets, stats->tx_bytes);
>
> - if (ipdevice->config == NULL)
> + if (ipdevice->config_ipv4 == NULL && ipdevice->config_ipv6 == NULL)
> return;
>
> ipdevice->rx_packets = stats->rx_packets;
> @@ -443,11 +513,18 @@ static void update_stats(struct connman_ipdevice
> *ipdevice,
> ipdevice->rx_dropped = stats->rx_dropped;
> ipdevice->tx_dropped = stats->tx_dropped;
>
> - __connman_service_notify(ipdevice->config,
> - ipdevice->rx_packets, ipdevice->tx_packets,
> - ipdevice->rx_bytes, ipdevice->tx_bytes,
> - ipdevice->rx_errors, ipdevice->tx_errors,
> - ipdevice->rx_dropped, ipdevice->tx_dropped);
> + if (ipdevice->config_ipv4)
> + __connman_service_notify(ipdevice->config_ipv4,
> + ipdevice->rx_packets,
> ipdevice->tx_packets,
> + ipdevice->rx_bytes, ipdevice->tx_bytes,
> + ipdevice->rx_errors,
> ipdevice->tx_errors,
> + ipdevice->rx_dropped,
> ipdevice->tx_dropped);
> + else
> + __connman_service_notify(ipdevice->config_ipv6,
> + ipdevice->rx_packets,
> ipdevice->tx_packets,
> + ipdevice->rx_bytes, ipdevice->tx_bytes,
> + ipdevice->rx_errors,
> ipdevice->tx_errors,
> + ipdevice->rx_dropped,
> ipdevice->tx_dropped);
> }
Here you could prepare a separate patch that would change the
__connman_service_notify() prototype to take a service instead of an ipconfig
as its first argument. Then you'd get the service from either your ipv4 or
ipv6 config and pass it to __connman_service_notify() here. The code would be
more readable.
> +static inline gint check_duplicate_address(gconstpointer a, gconstpointer b)
> +{
> + const struct connman_ipaddress *addr1 = a;
> + const struct connman_ipaddress *addr2 = b;
> +
> + if (addr1->prefixlen != addr2->prefixlen)
> + return addr2->prefixlen - addr1->prefixlen;
> +
> + return g_strcmp0(addr1->local, addr2->local);
> +}
> +
> void __connman_ipconfig_newaddr(int index, int family, const char *label,
> unsigned char prefixlen, const char *address)
> {
> @@ -614,26 +702,34 @@ void __connman_ipconfig_newaddr(int index, int family,
> const char *label,
> ipaddress->prefixlen = prefixlen;
> ipaddress->local = g_strdup(address);
>
> + if (g_slist_find_custom(ipdevice->address_list, ipaddress,
> + check_duplicate_address)) {
> + connman_ipaddress_free(ipaddress);
> + return;
> + }
> +
Those 2 chunks should definitely be part of a separate patch.
> ipdevice->address_list = g_slist_append(ipdevice->address_list,
> ipaddress);
>
> - connman_info("%s {add} address %s/%u label %s", ipdevice->ifname,
> - address, prefixlen, label);
> + connman_info("%s {add} address %s/%u label %s family %d",
> + ipdevice->ifname, address, prefixlen, label, family);
>
> - if (ipdevice->config != NULL) {
> - if (family == AF_INET6 && ipdevice->config->ipv6 != NULL)
> - connman_ipaddress_copy(ipdevice->config->ipv6->system,
> + if (family == AF_INET) {
> + if (ipdevice->config_ipv4 != NULL)
Please keep the same formatting: if (a && b) instead of:
if (a)
if (b)
> + connman_ipaddress_copy(ipdevice->config_ipv4->system,
> ipaddress);
> - else
> - connman_ipaddress_copy(ipdevice->config->system,
> +
> + } else if (family == AF_INET6) {
> + if (ipdevice->config_ipv6 != NULL)
> + connman_ipaddress_copy(ipdevice->config_ipv6->system,
> ipaddress);
> - }
> + } else
> + return;
>
> if ((ipdevice->flags & (IFF_RUNNING | IFF_LOWER_UP)) != (IFF_RUNNING |
> IFF_LOWER_UP))
> return;
>
> - if (g_slist_length(ipdevice->address_list) > 1)
> - return;
That is a bug, and should be part of a separate patch.
> @@ -859,19 +969,43 @@ const char *__connman_ipconfig_get_gateway(int index)
> if (ipdevice->ipv4_gateway != NULL)
> return ipdevice->ipv4_gateway;
>
> - if (ipdevice->config != NULL &&
> - ipdevice->config->address != NULL)
> - return ipdevice->config->address->gateway;
> + if (ipdevice->config_ipv4 != NULL &&
> + ipdevice->config_ipv4->address != NULL)
> + return ipdevice->config_ipv4->address->gateway;
> +
> + if (ipdevice->ipv6_gateway != NULL)
> + return ipdevice->ipv6_gateway;
> +
> + if (ipdevice->config_ipv6 != NULL &&
> + ipdevice->config_ipv6->address != NULL)
> + return ipdevice->config_ipv6->address->gateway;
>
> return NULL;
> }
>
> +int __connman_ipconfig_get_configs(int index,
> + struct connman_ipconfig **ipv4,
> + struct connman_ipconfig **ipv6)
So we probably don't need this routine. I see that you're calling it from the
provider creation code in service.c. There we could simply create both v4 and
v6 ipconfigs, the provider code will only handle ipv4 connection for now
anyway.
> @@ -998,7 +1121,9 @@ static void free_ipv6config(struct connman_ipconfig
> *ipconfig)
> */
> void connman_ipconfig_unref(struct connman_ipconfig *ipconfig)
> {
> - if (g_atomic_int_dec_and_test(&ipconfig->refcount) == TRUE) {
> + if (ipconfig &&
> + g_atomic_int_dec_and_test(&ipconfig->refcount) == TRUE) {
> +
Here again, this is a valid fix but should be separated from this patch.
> @@ -1098,7 +1222,10 @@ struct connman_ipconfig
> *connman_ipconfig_get_ipv6config(
> if (ipconfig == NULL)
> return NULL;
>
> - return ipconfig->ipv6;
> + if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV6)
> + return ipconfig;
> +
> + return NULL;
Same here: Valid fix, but irrelevant to this patch.
> @@ -1147,7 +1274,8 @@ void __connman_ipconfig_set_element_ipv6_gateway(
> struct connman_ipconfig *ipconfig,
> struct connman_element *element)
> {
> - element->ipv6.gateway = ipconfig->ipv6->address->gateway;
> + if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV6)
> + element->ipv6.gateway = ipconfig->address->gateway;
Ditto.
> @@ -1155,7 +1283,7 @@ void __connman_ipconfig_set_element_ipv6_gateway(
> * Set IPv4 and IPv6 gateway
> */
> int __connman_ipconfig_set_gateway(struct connman_ipconfig *ipconfig,
> - struct connman_element *parent)
> + struct connman_element *parent)
Irrelevant, please remove it from this patch.
> @@ -1385,6 +1548,9 @@ void __connman_ipconfig_append_ipv4(struct
> connman_ipconfig *ipconfig,
>
> DBG("");
>
> + if (ipconfig->type != CONNMAN_IPCONFIG_TYPE_IPV4)
> + return;
> +
Move this one to a separate patch.
> @@ -1421,6 +1587,9 @@ void __connman_ipconfig_append_ipv6(struct
> connman_ipconfig *ipconfig,
>
> DBG("");
>
> + if (ipconfig->type != CONNMAN_IPCONFIG_TYPE_IPV6)
> + return;
> +
Ditto.
> @@ -169,11 +168,6 @@ static void network_destruct(struct connman_element
> *element)
> g_free(network->address);
> g_free(network->identifier);
>
> - if (network->ipconfig) {
> - connman_ipconfig_unref(network->ipconfig);
> - network->ipconfig = NULL;
> - }
> -
Removing the ipconfig pointer is a good idea and makes sense. Please send a
separate patch for that.
> @@ -300,16 +294,25 @@ void connman_network_set_index(struct connman_network
> *network, int index)
> if (service == NULL)
> goto done;
>
> - ipconfig = __connman_service_get_ipconfig(service);
> + ipconfig = __connman_service_get_ip4config(service);
> +
> + DBG("index %d service %p ip4config %p", network->element.index,
> + service, ipconfig);
> +
> + if (network->element.index < 0 && ipconfig == NULL) {
>
> - if (network->element.index < 0 && ipconfig == NULL)
> - /*
> - * This is needed for plugins that havent set their ipconfig
> - * layer yet, due to not being able to get a network index
> - * prior to creating a service.
> - */
> - __connman_service_create_ipconfig(service, index);
> - else {
> + ipconfig = __connman_service_get_ip6config(service);
You probably meant __connman_service_get_ip4config(service);
> @@ -626,7 +648,7 @@ static void set_connected_manual(struct connman_network
> *network)
>
> service = __connman_service_lookup_from_network(network);
>
> - ipconfig = __connman_service_get_ipconfig(service);
> + ipconfig = __connman_service_get_ip4config(service);
I hope we will be able to merge the manual_ipv6_set() routine into this one.
We'll have to if we ever want to support basic v6 only networks.
> @@ -705,31 +727,39 @@ static gboolean set_connected(gpointer user_data)
> {
> struct connman_network *network = user_data;
> struct connman_service *service;
> - struct connman_ipconfig *ipconfig;
> - enum connman_ipconfig_method method;
> + struct connman_ipconfig *ipconfig_ipv4, *ipconfig_ipv6;
> + enum connman_ipconfig_method ipv4_method, ipv6_method;
>
> service = __connman_service_lookup_from_network(network);
>
> - ipconfig = __connman_service_get_ipconfig(service);
> + ipconfig_ipv4 = __connman_service_get_ip4config(service);
> + ipconfig_ipv6 = __connman_service_get_ip6config(service);
>
> - method = __connman_ipconfig_get_method(ipconfig);
> + DBG("service %p ipv4 %p ipv6 %p", service, ipconfig_ipv4,
> + ipconfig_ipv6);
>
> - DBG("method %d", method);
> + if (ipconfig_ipv4)
> + ipv4_method = __connman_ipconfig_get_method(ipconfig_ipv4);
> + else
> + ipv4_method = CONNMAN_IPCONFIG_METHOD_UNKNOWN;
I would be fine if we would have __connman_ipconfig_get_method() returning
CONNMAN_IPCONFIG_METHOD_UNKNOWN or CONNMAN_IPCONFIG_METHOD_OFF for a NULL
argument.
> @@ -1012,42 +1042,46 @@ int __connman_network_clear_ipconfig(struct
> connman_network *network,
> }
>
> int __connman_network_set_ipconfig(struct connman_network *network,
> - struct connman_ipconfig *ipconfig)
> + struct connman_ipconfig *ipconfig_ipv4,
> + struct connman_ipconfig *ipconfig_ipv6)
> {
> - struct connman_ipconfig *ipv6config;
> enum connman_ipconfig_method method;
> int ret;
>
> - ipv6config = connman_ipconfig_get_ipv6config(ipconfig);
> - method = __connman_ipconfig_get_method(ipv6config);
> - switch (method) {
> - case CONNMAN_IPCONFIG_METHOD_UNKNOWN:
> - case CONNMAN_IPCONFIG_METHOD_OFF:
> - break;
> - case CONNMAN_IPCONFIG_METHOD_FIXED:
> - case CONNMAN_IPCONFIG_METHOD_MANUAL:
> - ret = manual_ipv6_set(network, ipv6config);
> - if (ret != 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
> - return FALSE;
> + if (ipconfig_ipv6) {
The last comment would make this core much easier to read.
> @@ -669,12 +669,12 @@ void connman_provider_set_index(struct connman_provider
> *provider, int index)
> if (service == NULL)
> return;
>
> - ipconfig = __connman_service_get_ipconfig(service);
> + ipconfig = __connman_service_get_ip4config(service);
>
> if (ipconfig == NULL) {
> - __connman_service_create_ipconfig(service, index);
> + __connman_service_create_ip4config(service, index);
>
> - ipconfig = __connman_service_get_ipconfig(service);
> + ipconfig = __connman_service_get_ip4config(service);
It would look nicer if we would set both v6 and v4 indexes here, even though
the provider code only cares about v4 for now.
> static void append_ipv4config(DBusMessageIter *iter, void *user_data)
> {
> struct connman_service *service = user_data;
>
> - if (service->ipconfig != NULL)
> - __connman_ipconfig_append_ipv4config(service->ipconfig, iter);
> + if (service->ipconfig_ipv4 != NULL)
> + __connman_ipconfig_append_ipv4config(service->ipconfig_ipv4,
> iter);
>
Over 80 chars line here.
> static void append_ipv6config(DBusMessageIter *iter, void *user_data)
> {
> struct connman_service *service = user_data;
> - struct connman_ipconfig *ipv6config;
>
> - if (service->ipconfig == NULL)
> - return;
> -
> - ipv6config = connman_ipconfig_get_ipv6config(service->ipconfig);
> - if (ipv6config == NULL)
> - return;
> -
> - __connman_ipconfig_append_ipv6config(ipv6config, iter);
> + if (service->ipconfig_ipv6 != NULL)
> + __connman_ipconfig_append_ipv6config(service->ipconfig_ipv6,
> iter);
> }
Ditto.
> @@ -1224,6 +1223,8 @@ void __connman_service_notify(struct connman_ipconfig
> *ipconfig,
> struct connman_stats_data *data;
> int err;
>
> + DBG("ipconfig %p", ipconfig);
> +
Not really relevant to this patch.
> @@ -4158,16 +4267,38 @@ __connman_service_create_from_provider(struct
> connman_provider *provider)
>
> service->strength = 0;
>
> - service->ipconfig = connman_ipconfig_create(index);
> - if (service->ipconfig == NULL)
> + if (__connman_ipconfig_get_configs(index, &ipconfig_ipv4,
> + &ipconfig_ipv6) < 0)
> return service;
>
> - connman_ipconfig_set_method(service->ipconfig,
> + if (ipconfig_ipv4) {
> + service->ipconfig_ipv4 = connman_ipconfig_create(index,
> + CONNMAN_IPCONFIG_TYPE_IPV4);
> + if (service->ipconfig_ipv4 == NULL)
> + return service;
> +
> + connman_ipconfig_set_method(service->ipconfig_ipv4,
> CONNMAN_IPCONFIG_METHOD_MANUAL);
>
> - connman_ipconfig_set_data(service->ipconfig, service);
> + connman_ipconfig_set_data(service->ipconfig_ipv4, service);
> +
> + connman_ipconfig_set_ops(service->ipconfig_ipv4, &service_ops);
> + }
> +
> + if (ipconfig_ipv6) {
> + service->ipconfig_ipv6 = connman_ipconfig_create(index,
> + CONNMAN_IPCONFIG_TYPE_IPV6);
> + if (service->ipconfig_ipv6) {
> + connman_ipconfig_set_method(service->ipconfig_ipv6,
> + CONNMAN_IPCONFIG_METHOD_OFF);
> +
> + connman_ipconfig_set_data(service->ipconfig_ipv6,
> + service);
>
> - connman_ipconfig_set_ops(service->ipconfig, &service_ops);
> + connman_ipconfig_set_ops(service->ipconfig_ipv6,
> + &service_ops);
> + }
> + }
So as stated above, You can safely create both v4 and v6 pointers for a
provider service, no need for __connman_ipconfig_get_configs().
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
