Hi Lukas,
On Thu, Jun 20, 2013 at 08:26:47PM +0200, Lukas Tribus wrote:
> Hi Willy,
>
>
> > One is a fix (to->from) and can as well include the doc fix.
>
> Attaching the patch. I stole your explanation for the commit message, hope
> you don't mind :)
No of course, you're welcome.
> I'm thinking about this logic (pseudo code):
> #if defined(IP_TOS) && defined(IPV6_TCLASS)
> /* IP_TOS needs to be set for both families, because a AF_INET6
> socket may carries IPv4 traffic (when IPV6_V6ONLY is 0) */
> if (address-family == AF_INET || address-family == AF_INET6)
> setsockopt(fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos));
> if (address-family == AF_INET6)
> setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos, sizeof(tos));
> #endif
I don't like much mixing IPv4 and IPv6 together in the #ifdef,
because I don't know if IPV6_TCLASS is defined on all libcs,
especially old ones or embedded ones, where it could result in
disabling the IPv4 setting. I'd rather split that approximately
like this :
#if defined(IP_TOS)
if (address-family == AF_INET)
setsockopt(fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos));
if (address-family == AF_INET6) {
if (setsockopt(fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) ==
-1) {
#if defined(IPV6_TCLASS)
setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos,
sizeof(tos));
#endif
}
}
#endif
It would also avoid doing systematically two syscalls when the
first one succeeds.
> Is the address-family check needed anyway or is this a result of your
> intention to do this in v4 only?
I prefer to keep it because I don't know what these setsockopt can do on
unix sockets for example. And since we don't use exactly the same ones
depending on the family, it's better as well.
> Its hard to upset setsockopt, it will just return -1 when things don't make
> sense, nothing breaks:
>
> when family is AF_INET (a true ipv4 socket):
> setsockopt->IPV6_TCLASS returns -1
> setsockopt->IP_TOS returns 0
>
> when family is AF_INET6 (either a true ipv6 or a v4 mapped socket)
> setsockopt->IPV6_TCLASS returns 0
> setsockopt->IP_TOS returns 0
>
> when family is a unix-socket obviously:
> setsockopt->IPV6_TCLASS returns -1
> setsockopt->IP_TOS returns -1
I must say I don't much like making syscalls return errors when we can
avoid them, because not only this means they're called for no reason,
but more specifically they make debugging more difficult (eg: strace -c).
> With the pseudo code example above, setsockopt would always be successful,
> so this should be most portable and the corner case of a v4-in-a-v6-mapped
> socket would be addressed.
I already forgot about this one :-)
Then maybe why not :
#if defined(IP_TOS)
if (address-family == AF_INET || address-family == AF_INET6) {
if (setsockopt(fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) ==
-1) {
#if defined(IPV6_TCLASS)
setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos,
sizeof(tos));
#endif
}
}
#endif
This would only call the second one if the first fails, which according
to your tests should not happen.
Or you can even put that more elegantly in an inline function :
static inline void inet_set_tos(int fd, sa_family_t family, int tos)
{
if (family != AF_INET && family != AF_INET6)
return;
#if defined(IP_TOS)
if (setsockopt(fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) == 0)
return;
#endif
#if defined(IPV6_TCLASS)
if (setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &tos, sizeof(tos) == 0)
return;
#endif
}
What do you think ?
Best regards,
Willy
