> This looks fine, but you forgot the "fixed" method. Remember that our
Ok, I will add it. It is simple. ;-)
> 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.
Ok, make sense
>
> > 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?
You have helped me answer it. Now we only use IPV6_GATEWAY and need not bother
with others.
> > struct gateway_data {
> > int index;
> > char *gateway;
> > + char *ipv6_gateway;
>
> Make it ipv4_gateway and ipv6_gateway.
OK
>
> > -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.
I think I have checked at both sides.
it is safe to also check here, you know in the end we may also get address from
oFono or dhcp-lib.
>
> 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.
In fact it is not a big issue, but I think from the integration of one
function. It makes sense to check the parameter. ;-)
> > + 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.
OK.
> > +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.
OK, but the function of perror is attractive, it is helpful for us to debug the
issue.
I think we can have another patch to add perror feature to 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?
The prefix of host is useless here. The prefix of host should be 128, So here,
just remove the "/prefix"
>
> > #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.
Just as describe in my mail, it is just for your reference, it will be removed
definitely. :-)
>
> > +
> > + 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.
That also make sense to me.
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
