Hi Denis,
> So during OSTS Samuel, Marcel and I sat down and tried to
> figure out the
> IPv6 stuff. Based on this discussion and your implementation
> I pushed a
> series of patches implementing IPv6 and dual-stack contexts. I have
> taken (and later re-worked) some of your patches so you get
> credit here
> as well.
Thanks for pushing the patches. I notice that these are based on my initial set
of patches rather than the later ones. A few comments, since I had some reasons
for the changes I did in later patches.
I notice that your version of the patches are not forming the IPv6 link-local
address and configuring it on the network interface. That's ok, as long as
connman takes care of it, but it does mean that Ethernet interfaces, which
always have a link-local address, will autoconfigure immediately while
point-to-point interfaces will only autoconfigure when connman sets the
link-local address on them. We talked about this with Marcel and at the time
concluded that it would make more sense to keep things consistent by having
oFono configure the link-local address on the point-to-point interfaces. I had
this implemented in my later patch sets.
A few comments on the driver API:
void ofono_gprs_context_set_ipv4_address(struct ofono_gprs_context *gc,
const char *address,
gboolean static_ip);
What's the expected behaviour if this is called with a valid IP address and
static_ip = FALSE? I think you could just drop the boolean flag and assume a
statically configured address if this method is called by the driver, otherwise
do DHCP.
void ofono_gprs_context_set_ipv4_dns_servers(struct ofono_gprs_context
*gc,
const char **dns);
void ofono_gprs_context_set_ipv6_dns_servers(struct ofono_gprs_context
*gc,
const char **dns);
I would propose to have just a single method for setting all DNS servers.
Having separate lists for IPv4 and IPv6 DNS servers made sense in my first
patch set, because a dual context could be emulated with separate IPv4 and IPv6
contexts and those DNS servers might have been behind different network
interfaces. However, now this just creates additional complexity for the
drivers. A dual context will get a list of DNS server addresses, which may
contain IPv4 addresses, IPv6 addresses or both. Now the driver has to sort them
into two separate lists for IPv4 and IPv6. Note that you can make A and AAAA
queries to any server, so there is no particular reason to separate the lists
based on address family.
void ofono_gprs_context_set_ipv6_prefix_length(struct
ofono_gprs_context *gc,
unsigned char length);
void ofono_gprs_context_set_ipv6_gateway(struct ofono_gprs_context *gc,
const char *gateway);
I'm not sure these are really needed, which is why I dropped these from
subsequent patches. This information is not received from the cellular network
as part of context activation signalling. On-link prefixes, routes and default
gateways are received as part of the standard IPv6 stateless address
autoconfiguration when the interfaces is brought up. The only reason to have
these would if a specific modem with a virtual ethernet interface deviates from
the standard address configuration practises for some reasons.
Current USB modem sticks don't seem to have IPv6 support, so I'd propose to
just drop these and add an API later if it turns out to be necessary. If USB
sticks do this propertly, they'll just proxy router advertisements and neighbor
discovery messages over the virtual ethernet interface and any additional
address configuration settings won't be needed.
If you decide to keep these, prefix length should probably default to 64 and be
always shown in the settings.
> These are highly experimental and have not received much
> testing (since
> I don't really have any facilities to do so). So please look
> and let me
> know if something isn't working as intended.
I'm not able to test dual context but IPv6 seems to work with isimodem. I did
notice that the context settings allocated in assign_context() are leaked if
context activation fails. Easy enough to fix, though:
iff --git a/src/gprs.c b/src/gprs.c
index 00f6d6d..068aaf3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -322,11 +322,13 @@ static gboolean assign_context(struct pri_context *ctx)
return FALSE;
}
+static void context_settings_free(struct context_settings *settings);
static void release_context(struct pri_context *ctx)
{
if (ctx == NULL || ctx->gprs == NULL || ctx->context_driver == NULL)
return;
+ context_settings_free(ctx->context_driver->settings);
gprs_cid_release(ctx->gprs, ctx->context.cid);
ctx->context.cid = 0;
ctx->context_driver->inuse = FALSE;
Regards,
MikaL
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
