Hi Martin, actually inline patches are still simpler to review ;)
> From d8185f8c6a8f517ee6bb7ca2178fe3aac2b9cf97 Mon Sep 17 00:00:00 2001 > From: Martin Xu <[email protected]> > Date: Tue, 1 Jun 2010 15:30:08 +0800 > Subject: [PATCH] initial commit of static ipv6 support > > --- > doc/service-api.txt | 25 ++++ > include/element.h | 11 ++ > include/ipconfig.h | 12 ++- > include/property.h | 1 + > src/connection.c | 23 +++- > src/connman.h | 13 ++- > src/element.c | 7 + > src/inet.c | 309 > +++++++++++++++++++++++++++++++++++++++++++++++++ > src/ipconfig.c | 265 ++++++++++++++++++++++++++++++++++++++++-- > src/ipv4.c | 13 ++- > src/network.c | 57 +++++++++- > src/service.c | 120 ++++++++++++++++++-- > test/set-ipv6-address | 29 +++++ > 13 files changed, 852 insertions(+), 33 deletions(-) > create mode 100755 test/set-ipv6-address > > diff --git a/doc/service-api.txt b/doc/service-api.txt > index a3e3c10..ee7dce9 100644 > --- a/doc/service-api.txt > +++ b/doc/service-api.txt > @@ -353,6 +353,31 @@ Properties string State [readonly] > until the new configuration has been successfully > installed. > > + dict IPv6 [readonly] > + > + string Method [readonly] > + > + Possible values are "manual" and "off". > + > + string Address [readonly] > + > + The current configured IPv6 address and > prefix. > + > + string Gateway [readonly] > + > + The current configured IPv6 gateway. > + > + dict IPv6.Configuration [readwrite] > + > + Same values as IPv6 property. The IPv6 represents > + the actual system configuration while this allows > + user configuration. > + > + Changing these settings will cause a state change > + of the service. The service will become unavailable > + until the new configuration has been successfully > + installed. > + This looks fine, but you forgot the "fixed" method. Remember that our initial target is IPv6 for cellular networks and these are special static addresses with the "fixed" method. So we clearly tell the user interface that they can't be changed. > + > + struct { > + enum connman_ipconfig_method method; > + gchar *address; > + gchar *gateway; > + gchar *network; > + gchar *broadcast; > + gchar *nameserver; > + gchar *timeserver; > + gchar *pac; > + } ipv6; > }; Don't bother with details like broadcast, nameserver, timeserver and pac. We should not be handling these in the initial patch. And most likely we need to handle these differently anyway. So for example the Nameservers configuration can be used for IPv4 and IPv6. It is not related to the actual IPv6 settings and we wanna keep them separated. Same will also apply to the timeserver and the proxy setting in the end. So for now, just leave it out. > diff --git a/include/property.h b/include/property.h > index c7f2a7b..bb71d2b 100644 > --- a/include/property.h > +++ b/include/property.h > @@ -48,6 +48,7 @@ enum connman_property_id { > CONNMAN_PROPERTY_ID_IPV4_NAMESERVER, > CONNMAN_PROPERTY_ID_IPV4_TIMESERVER, > CONNMAN_PROPERTY_ID_IPV4_PAC, > + CONNMAN_PROPERTY_ID_IPV6_GATEWAY, > }; Why only gateway? > struct gateway_data { > int index; > char *gateway; > + char *ipv6_gateway; Make it ipv4_gateway and ipv6_gateway. > -static struct gateway_data *add_gateway(int index, const char *gateway) > +static struct gateway_data *add_gateway(int index, const char *gateway, > + const char *ipv6_gateway) Also please do ipv4_gateway and ipv6_gateway here. > +int connman_inet_set_ipv6_address(int index, > + struct connman_ipaddress *ipaddress) > +{ > + int sk, ret; > + struct in6_ifreq ifr6; > + > + DBG("index %d ipaddress->local %s", index, ipaddress->local); > + > + if (ipaddress->local == NULL) > + return 0; > + > + /* Check IPV6 address type */ > + if (strchr(ipaddress->local, ':') == NULL) { > + ret = -1; > + goto out; > + } Sounds like a sensible check, but why here. I would expect this check to be done from user input. And then return invalid format. We might not be checking the user input closely enough. That might could be an extra patch to also add extra checks for IPv4 addresses. > + if (ipaddress->prefixlen < 0 || ipaddress->prefixlen > 128) { > + ret = -1; > + goto out; > + } Same here. I would expect to do that way earlier. > + sk = socket(PF_INET6, SOCK_DGRAM, 0); > + if (sk < 0) { > + ret = -1; > + goto out; > + } > + > + memset(&ifr6, 0, sizeof(ifr6)); > + > + if (inet_pton(AF_INET6, ipaddress->local, &ifr6.ifr6_addr) <= 0) { > + ret = -1; > + goto close; > + } > + > + ifr6.ifr6_ifindex = index; > + ifr6.ifr6_prefixlen = ipaddress->prefixlen; > + > + ret = ioctl(sk, SIOCSIFADDR, &ifr6); > + if (ret < 0) > + goto close; > + > + ret = 0; > +close: > + close(sk); > +out: > + if (ret < 0) > + perror("Set IPV6 address error"); > + > + return ret; > +} As a general rule we are using err for error return results. > +int connman_inet_clear_ipv6_address(int index, const char *address, > + int prefix_len) > +{ > + struct in6_ifreq ifr6; > + int sk, ret; > + > + DBG("index %d address %s prefix_len %d", index, address, prefix_len); > + > + /* Check IPV6 address type */ > + if (strchr(address, ':') == NULL) { > + ret = -1; > + goto out; > + } > + > + if (prefix_len < 0 || prefix_len > 128) { > + ret = -1; > + goto out; > + } Same comments as above. > + memset(&ifr6, 0, sizeof(ifr6)); > + > + if (inet_pton(AF_INET6, address, &ifr6.ifr6_addr) <= 0) { > + ret = -1; > + goto out; > + } > + > + ifr6.ifr6_ifindex = index; > + ifr6.ifr6_prefixlen = prefix_len; > + > + sk = socket(PF_INET6, SOCK_DGRAM, 0); > + if (sk < 0) { > + ret = -1; > + goto out; > + } > + > + ret = ioctl(sk, SIOCDIFADDR, &ifr6); > + if (ret < 0) > + goto close; > + > + ret = 0; > +close: > + close(sk); > +out: > + if (ret < 0) > + perror("Clear IPV6 address error"); perror seems to be wrong here. Lets use connman_error. > + > + return ret; > +} Please use err (see above). > +int connman_inet_add_ipv6_host_route(int index, const char *host) > +{ > + struct in6_rtmsg rt; > + int sk, ret; > + char *c, *address = g_strdup(host); > + > + DBG("index %d host %s", index, host); > + > + if (host == NULL) > + return 0; > + > + c = strchr(address, '/'); > + if (c != NULL) > + *c = 0; What is this checking for. Are you trying to squeeze two things in one string? > #include "connman.h" > +#if 0 > +struct _ipconfig { > + enum connman_ipconfig_type type; > + > + const struct connman_ipconfig_ops *ops; > + void *ops_data; > + > + enum connman_ipconfig_method method; > + > + struct connman_ipaddress *address; > + struct connman_ipaddress *system; > +}; > > struct connman_ipconfig { > gint refcount; > int index; > + struct _ipconfig ipv4; > + struct _ipconfig ipv6; > +} > +#endif Please don't do that. I really don't like ifdefs and this seems to be more like a leftover from some testing session. > + > + DBG("prefix_len %d address %s gateway %s", > + prefix_len, address, gateway); > + > + ipaddress->prefixlen = prefix_len; > + > +// ipaddress->type = CONNMAN_IPCONFIG_TYPE_IPV6; Leftover? > + > + g_free(ipaddress->local); > + ipaddress->local = g_strdup(address); > + > + g_free(ipaddress->gateway); > + ipaddress->gateway = g_strdup(gateway); > + > + return 0; > +} > + > +void connman_ipaddress_set_ipv4(struct connman_ipaddress *ipaddress, > const char *address, const char *netmask, const char *gateway) > { > if (ipaddress == NULL) > @@ -121,6 +208,8 @@ void connman_ipaddress_set(struct connman_ipaddress > *ipaddress, > else > ipaddress->prefixlen = 32; > > +// ipaddress->type = CONNMAN_IPCONFIG_TYPE_IPV4; > + What about this? > +void __connman_ipconfig_append_ipv6config(struct connman_ipconfig *ipconfig, > + DBusMessageIter *iter) > +{ > + char *address; > + const char *str; > + > + str = __connman_ipconfig_method2string(ipconfig->method); > + if (str == NULL) > + return; > + > + connman_dbus_dict_append_basic(iter, "Method", DBUS_TYPE_STRING, > &str); > + > + switch (ipconfig->method) { > + case CONNMAN_IPCONFIG_METHOD_UNKNOWN: > + case CONNMAN_IPCONFIG_METHOD_OFF: > + case CONNMAN_IPCONFIG_METHOD_FIXED: > + case CONNMAN_IPCONFIG_METHOD_DHCP: > + return; > + case CONNMAN_IPCONFIG_METHOD_MANUAL: > + break; > + } We should also support FIXED. > + if (ipconfig->address == NULL) > + return; > + > + address = g_strdup_printf("%s/%d", ipconfig->address->local, > + ipconfig->address->prefixlen); > + if (ipconfig->address->local != NULL) > + connman_dbus_dict_append_basic(iter, "Address", > + DBUS_TYPE_STRING, &address); > + g_free(address); So I am thinking that the Prefixlen should be exposed as separate field. Same as for example MacOS is currently doing it. Meaning we are having Address, PrefixLength and Gateway. Regards Marcel _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
