tasklwIP - A Lightweight TCP/IP stack - Tasks: task #14247, socket API: implement recvmsg...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #14247: socket API: implement recvmsg msg_control support

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Wed 23 Nov 2016 12:01:25 PM UTC  
 
Category: NoneShould Start On: Wed 23 Nov 2016 12:00:00 AM UTC
Should be Finished on: Wed 23 Nov 2016 12:00:00 AM UTCPriority: 5 - Normal
Status: NonePrivacy: Public
Percent Complete: 90%Assigned to: Joel Cunningham <jcunningham>
Open/Closed: OpenPlanned Release: None
Effort: 0.00

(Jump to the original submission Jump to the original submission)

Thu 27 Jul 2017 06:57:26 PM UTC, comment #29:

No, it's fine like that. I'll just remove the "in progress" marker then. I'm trying to get a hold on what's missing for 2.1.0 and reading your post, it doesn't make sense to wait for this task to be finished.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 26 Jul 2017 09:34:02 PM UTC, comment #28:

No update. I'd still like to get the IPv6 version of IP_PKTINFO implemented, but haven't found time to get it done. We can move that to a new issue if you'd like to close this one out. recvmsg with control messages and IP_PKTINFO is ready to be used

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 26 Jul 2017 07:41:01 PM UTC, comment #27:

This is marked as "in progress", any update here or should we remove the marker?

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 26 Apr 2017 01:46:37 PM UTC, comment #26:

Simon,

Thanks for clarifying the error. I'm also fine with the added void . RFC says the type should be unsigned char , but void should be fine since we are always casting to the real type (for CMSG_DATA)

I've also pushed a fix for ALIGN_H and ALIGN_D to address the potential problem on 16-bit arches where MEM_ALIGN is 2, but we need to be aligning to 4. This change also ensures that there is no unaligned access even for systems that support it (x86)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 26 Apr 2017 06:45:06 AM UTC, comment #25:

I've pushed a change adding a cast via void pointer (which is what Linux does :)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 26 Apr 2017 06:19:03 AM UTC, comment #24:

Whatever the problem with ALIGN_H and ALIGN_D is, the compiler error would not be fixed by changing that. The compiler just sees

It's the direct cast from u8_t pointer to struct pointer that is "bad". Using LWIP_ALIGNMENT_CAST() should fix this, but I'm not sure where to add that or what the standard thing to do here is. We should probably return a void pointer here instead of an u8_t pointer.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 25 Apr 2017 07:10:07 PM UTC, comment #23:

Travis CI for clang is showing the follow error:

I think we might have a problem with using MEM_ALIGNMENT for ALIGN_H and ALIGN_D.

I'm somewhat confused why the error is generated for clang on x86_64. I think fno-sanitize=alignment is forcing no unaligned access on x86_64, right? If so, why is MEM_ALIGNMENT set to 1 for unix port (ports\unix\lib\lwipopts.h or ports\unix\minimal\lwipopts.h, not sure which the unit test is using)?

Second, I could set ALIGN_H and ALIGN_D to 4 which would be correct for architectures with 4 byte alignment, but what about architectures with 2 byte alignment? I don't have much experience with these architectures. cmsg_len (type socklen_t) could end up being a u32 and in a 2 byte architecture, does this need to start on a 4 byte boundary? If so, then using MEM_ALIGNMENT would not work.

If we ever add a cmsg data type that is 64-bits, we'll need to up the alignment to 8, even though MEM_ALIGNMENT would still be 4 on architectures like ARM.

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 25 Apr 2017 02:49:02 PM UTC, comment #22:

Patch committed in 2f117add7a8af07e483e1ee18af559f3e4e0d389. Updating progress to 90%. Remaining 10% is for IPv6 version of IP_PKTINFO (hopefully I can provide a patch later in the week)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 25 Apr 2017 02:05:41 PM UTC, comment #21:

> It seemed minor to me since it was only a single check of: if (msg->msg_contrllen).


I rather thought about the 2nd indrection we get in standard receive because we create an (unused) struct msghdr. It's not a big deal, but if we find a decent way to separate it...

> Do you want me to go ahead and commit?


Yes :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 25 Apr 2017 02:01:51 PM UTC, comment #20:

We could move the msg_control code out of lwip_recvfrom_udp_raw() if we change the function so it can output the netbuf (which would then be processed in lwip_recvmsg). It seemed minor to me since it was only a single check of: if (msg->msg_contrllen).

Do you want me to go ahead and commit?

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 25 Apr 2017 08:20:27 AM UTC, comment #19:

That looks much better. Although I'm still not convinced we need the msg_control code when calling lwip_recvfrom, but let's optimize that later...

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 25 Apr 2017 04:45:41 AM UTC, comment #18:

New patch moves the flag to the netconn layer. The netconn layer flag + using the flag for the existing LWIP_NETBUF_RECVINFO feature resulted in a much cleaner implementation, thanks for the suggestion :)

I went with defaulting netconn_alloc() to set NETCONN_FLAG_PKTINFO and then lwip_socket() to clear the flag. This should retain the previous behavior when LWIP_NETBUF_RECVINFO was enabled

(file #40488)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 19 Apr 2017 08:06:36 PM UTC, comment #17:

Ahh, now I see what you meant about ' enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)' that way we could maintain the existing LWIP_NETBUF_RECVINFO behavior.

I'll work on a new patch!

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 19 Apr 2017 07:46:46 PM UTC, comment #16:

Sorry for not being clear enough :-)
Yes, we would need the check in recv_udp. And we would need the netconn->flag for LWIP_NETBUF_RECVINFO (!=0). But we would store the info only if the flag PKTINFO is enabled in the netconn.
And that could be enabled by default to leave netconn behaviour as it was, and be disabled for sockets. But in the end, it's probably good enough to just disable it by default for all netconns & sockets and provide an API to enable it...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 19 Apr 2017 07:40:20 PM UTC, comment #15:

Thanks for the suggestion, I had also thought of doing a socket/netconn level flag, let me make sure I understand all the suggestions

> leave LWIP_NETBUF_RECVINFO as it was


I would still need to utilize the logic in recv_udp to store the address and the toaddr storage in netbuf. Are you suggesting that I enable the toaddr storage all the time? If I have access to toaddr and flags, I wouldn't need to do anything with LWIP_NETBUF_RECVINFO

> enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)


Add API to enable/disable PKTINFO. If enabled at the netconn layer, toaddr and port are filled out in the netbuf

> disable NETCONN_FLAG_PKTINFO for sockets


By this you just mean it's off by default for sockets? I would also think it would be off by default for netconns

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 19 Apr 2017 07:13:55 PM UTC, comment #14:

The SOF_PKTINFO flag is better off in struct netconn anyway. It is not used in core code at all.
Given that we could:
- make it NETCONN_FLAG_PKTINFO
- leave LWIP_NETBUF_RECVINFO as it was
- enable NETCONN_FLAG_PKTINFO for netconns (add a function/define for that!)
- disable NETCONN_FLAG_PKTINFO for sockets
- enable NETCONN_FLAG_PKTINFO if enabled via setsockopt(IP_PKTINFO)
- output PKTINFO if the netbuf has that information

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 19 Apr 2017 07:01:00 PM UTC, comment #13:

> thing that looks strange is accessing the netconn's pcb without locking. Even if it's only a read access, chances are that the pcb already got deallocated. If not now, this might happen in the future. Isn't it enough to check the flag in the netbuf in this case?

Thanks for reminding me that PCB could be deallocated. I think I can remove the check because your suggestion of relying on buf->flags having NETBUF_FLAG_DESTADDR set is enough.

I was thinking down the road when we add the IPv6 version, NETBUF_FLAG_DESTADDR would be set regardless of which version of PKTINFO socket option was set, but maybe checking the IP type via IP_IS_V4 or _IS_V6 is enough to determine which cmsg to output. Maybe I can also check the domain in setsockopt and error if IP_PKTINFO is mixed with AF_INET6 (and vice versa for AF_NET with IPv6 option)

> Shouldn't the new feature work with LWIP_NETBUF_RECVINFO==1, too?

I could make it work with this configuration as well, but was limiting the scope of the change. The netbuf flags field would need to be enabled this configuration (currently it's not). Also in relation to the above point about not accessing the PCB and checking NETBUF_FLAG_DESTADDR, with NETBUF_RECVINFO == 1, the flag would always bet set regardless of the IP_PKTINFO option, which would mean if a cmsg was provided during recvmsg, but IP_PKTINFO was off, the stack would still output PKTINFO in the cmsg. Maybe this isn't a big deal since LWIP_NETBUF_RECVINFO == 1 always outputs the address

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 18 Apr 2017 08:00:48 PM UTC, comment #12:

Looks good. I can't comment on the corectness of the socket.h/.c additions though (CMSG...) without reading into the specs. But I guess that doesn't really matter. If it should be wrong, we can always fix it later.

I have two questions/comments though:
1) A thing that looks strange is accessing the netconn's pcb without locking. Even if it's only a read access, chances are that the pcb already got deallocated. If not now, this might happen in the future. Isn't it enough to check the flag in the netbuf in this case?
2) Shouldn't the new feature work with LWIP_NETBUF_RECVINFO==1, too?

Simon Goldschmidt <goldsimon>
Project Administrator
Sat 15 Apr 2017 05:08:07 PM UTC, comment #11:

Attached is patch implementing CMSG infrastructure (only for recvmsg) and IP_PKTINFO socket option (including unit test)

I have a couple of things to discuss about the change, patch contains current approach:

1. For struct in_pktinfo, I went with the netbsd usage rather than Linux. My thoughts here are that Linux has the additional ipi_spec_dst, which I believe will be set to the source address of the interface that received the packet. Since this was going to be an additional lookup once we are outside of the LwIP core context, it didn't seem necessary. Applications can still get this address from the interface index (we can implement SIOCGIFADDR if needed)

2. I re-used the LWIP_NETBUF_RECVINFO feature (rather than create a new option) to record the destination when the datagram is received, but I made a modification to the option. I added a new subset of behavior. If LWIP_NETBUF_RECVINFO is 2, the destination and port are only recorded when IP_PKTINFO is enabled. This preserves the old API behavior of LWIP_NETBUF_RECVINFO == 1 and doesn't suffer the performance hit of recording on every received datagram. I also enabled the netbuf flags for this case so we can check that the address was recorded since it's now dependent on IP_PKTINFO being enabled

3. I added an IP level so_option for PKTINFO (called SOF_PKTINFO). This differs a little from the other options because it doesn't directly map from the SO_ equvialent because IP_PKTINFO if an IP level option. I didn't see a better way to record this setting without introducing a separate flag

4. I changed lwip_recvfrom_udp_raw() to take struct msghdr because I need msg_control, msg_controllen, msg_flags and almost all members of the msghdr were being passed individually. This also means that lwip_recvfrom() is more or less just calling recvmsg because it sets up struct msghdr

Thanks :)

(file #40412)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 13 Apr 2017 03:39:42 PM UTC, comment #10:

RFC 3542, section 20 contains the specification for control messages (AKA Ancillary Data):

https://tools.ietf.org/html/rfc3542#section-20

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Fri 17 Mar 2017 10:22:53 PM UTC, comment #9:

Socket unit tests now utilize recvmsg for both UDP and TCP. So far no bugs in the implementation :)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 16 Mar 2017 10:21:09 PM UTC, comment #8:

Awesome, great :) I'll start playing with it. I can work on moving my sendmsg tests and convert them to using loopback

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 16 Mar 2017 09:57:19 PM UTC, comment #7:

> Do we have any mechanisms for testing the API yet?


I just tried to get started with this, have a look at test/unit/api and test/unit/arch.

I'd be happy to see more tests there, feel free to add them. That would be a much better place than contrib/apps/socket_examples (where the remote host is missing)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 14 Mar 2017 06:44:20 PM UTC, comment #6:

> Do we have any mechanisms for testing the API yet?


Unfortunately not. I'd love to have unit tests for sockets and netconn, but that would need callback driven test implementation of the sys_arch layer.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 14 Mar 2017 06:55:42 AM UTC, comment #5:

That would be great. I haven't started and don't know when I would have...

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 13 Mar 2017 10:09:57 PM UTC, comment #4:

I can give adding IP_PKTINFO and CMSG support a go if you hadn't already started on it. I plan on using the feature in my products and have also implemented these APIs in a proprietary stack a couple years back.

Do we have any mechanisms for testing the API yet? A longer term project, but maybe a sockets unit test would be helpful (we've been making more changes in sockets since 2.0.0 and there's probably some bugs)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Mon 13 Mar 2017 09:34:19 PM UTC, comment #3:

The actual recvmsg function is implemented, however, IP_PKTINFO/in_pktinfo is still missing (msg_control is not touched at all).

Feel free to add patches here :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 Nov 2016 03:06:28 PM UTC, comment #2:

The IPv6 version of all this stuff is separate and (fortunately) standardized in RFC 3542.

David van Moolenbroek <dcvmoole>
Wed 23 Nov 2016 02:51:25 PM UTC, comment #1:

Glad to see this as a task, recvmsg has been on my backlog for a while :)

I believe there is no open group standards for the message types and it ends up being fairly platform dependent.

Linux, NetBSD, and Windows use IP_PKTINFO, but struct in_pktinfo is different on the platforms.

Linux
NetBSD
Windows

FreeBSD has IP_RECVDSTADDR

For LwIP, I guess we get to pick which option and if IP_PKTINFO, the definition of struct in_pktinfo

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 23 Nov 2016 12:01:25 PM UTC, original submission:

Together with IP_PKTINFO option (is this standardized?), recvmsg() can give us the destination address of UDP datagrams received on an ANY socket...

Simon Goldschmidt <goldsimon>
Project Administrator

 

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by dcvmoole (Posted a comment)
  • -unavailable- added by jcunningham (Posted a comment)
  • -unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 27 Jul 2017 06:57:26 PM UTCgoldsimonStatusIn Progress=>None
    Wed 26 Apr 2017 06:19:35 AM UTCgoldsimonPercent Complete50%=>90%
    Tue 25 Apr 2017 07:10:07 PM UTCjcunninghamPercent Complete90%=>50%
    Tue 25 Apr 2017 02:49:02 PM UTCjcunninghamPercent Complete50%=>90%
    Tue 25 Apr 2017 04:45:41 AM UTCjcunninghamAttached File-=>Added 0001-sockets-task-14247-add-CMSG-and-IP_PKTINFO-conn-flag.patch, #40488
    Sat 15 Apr 2017 05:08:07 PM UTCjcunninghamAttached File-=>Added 0001-sockets-task-14247-add-CMSG-and-IP_PKTINFO.patch, #40412
    Thu 13 Apr 2017 02:42:59 PM UTCjcunninghamAssigned toNone=>jcunningham
    Mon 13 Mar 2017 09:34:19 PM UTCgoldsimonStatusNone=>In Progress
      Percent Complete0%=>50%
      Summarysocket API: implement recvmsg=>socket API: implement recvmsg msg_control support

    Back to the top


    Powered by Savane 3.1-cleanup1