Re: Plan for improving IP_PKTINFO socket option handling

[christos%astron.com@localhost (Christos Zoulas)]

In article <m2shbrc4zm.fsf%thuvia.hamartun.priv.no@localhost>,
Tom Ivar Helbekkmo  <tih%hamartun.priv.no@localhost> wrote:
>-=-=-=-=-=-
>
>I've got everything working, now, and ready for others to take a look
>at.  I've tested all functionality, and am quite happy with it.  I'm
>attaching the patch for perusal and criticism.
>
>I gave in to a little bit of feature creep: I included source level
>compatibility with FreeBSD, as adding the IP_SENDSRCADDR control message
>was so quick and easy to do while I was in there anyway.
>
>Binary compatibility with existing applications is maintained: I've
>verified that existing compiled binaries continue to work as before.
>
>There are a couple of little things I'd like opinions on, though.  More
>on that below.
>
>> 1) "#define ipi_spec_dst ipi_addr" in <netinet/in.h>
>
>Done.  This alone makes lots of software compile that didn't.
>
>> 2) Change the IP_RECVPKTINFO option to control the generation of
>>    IP_PKTINFO control messages, the way it's done in Solaris.
>
>Done.  Using IP_PKTINFO will still work, though -- see below.
>
>> 3) Remove the superfluous IP_RECVPKTINFO control message.
>
>Done.  It won't be missed -- nothing has ever used it.
>
>> 4) Change the IP_PKTINFO option to do different things depending on
>>    the parameter it's supplied with:
>>    - If it's sizeof(int), assume it's being used as in Linux:
>>      - If it's non-zero, turn on the IP_RECVPKTINFO option.
>>      - If it's zero, turn off the IP_RECVPKTINFO option.
>>    - If it's sizeof(struct in_pktinfo), assume it's being used as in
>>      Solaris, to set a default for the source interface and/or
>>      source address for outgoing packets on the socket.
>
>Done, but only for setsockopt(2).  The relevant kernel code knows the
>size of the data supplied, and can differentiate as described, and I do
>that.  For getsockopt(2), however, it's not so simple.  The size is not
>known (sopt->sopt_size is 0 during the IP socket option processing in
>ip_ctloutput() in sys/netinet/ip_output.c, which I'm tempted to call a
>bug), and thus I have no idea whether the caller expects an int or a
>struct in_pktinfo returned.  For now, I've punted, and just return the
>int that Linux source code, and existing NetBSD applications, expect.
>Comments and suggestions are welcome.

This came up in a different discussion; we should pass the size around...

>
>Another thing I'm unsure of here: when the application uses IP_PKTINFO
>to set a default source address for outgoing packets through a wildcard
>bound socket, I need a place to keep that address.  I decided to add a
>new member to the PCB structure for this purpose.  If that's stupid, and
>there's something else I should have done, someone please enlighten me.

I guess that's fine for now.


Great, minor nits :-)

+			error = sockopt_get(sopt, &pktinfo, sizeof(struct in_pktinfo));
+			if (!error) {

Early break here:
			if (error)
				break;

To avoid indent...

+				/* Solaris compatibility */
+				if (pktinfo.ipi_ifindex) {
+					struct ifnet *ifp;
+					struct in_ifaddr *ia;
@@ -1449,11 +1487,25 @@

I'd use sizeof(pktinfo) here:

 			if (cm->cmsg_len != CMSG_LEN(sizeof(struct in_pktinfo)))
 				return EINVAL;
 
-			pktinfo = (struct in_pktinfo *)CMSG_DATA(cm);
-			error = ip_pktinfo_prepare(pktinfo, pktopts, flags,

And write this:
+			memcpy(&pktinfo, (struct in_pktinfo *)CMSG_DATA(cm),
+			       sizeof(struct in_pktinfo));

as:
			memcpy(&pktinfo, CMSG_DATA(cm), sizeof(pktinfo));

christos