Skip to content

Conversation

@pfalcon
Copy link
Contributor

@pfalcon pfalcon commented May 11, 2017

This shows the very initial steps on moving BSD Sockets API (prototyped out of tree) into the tree.

Things to decide/ack:

  • Location. While later we may need tighter integration with subsys/net/ip/, so far everything fits nicely into subsys/net/lib/sockets/.
  • Prefix to use for functions. As was discussed during RFC, there should be prefix, and optionally a header should #define pure POSIX names.
  • Header name/location.
  • Changes to net_context and friends. As can be seen, the only addition is a per-socket packet/connection queue. There's a recast of existing bit, and fairly speaking, I need to track more detailed context status, still looking how to do that, if anything there're whole 4 bytes of user_data pointer.

@pfalcon
Copy link
Contributor Author

pfalcon commented May 11, 2017

@jukkar , @tbursztyka , @Likailiu, @simple2017 , @mike-scott : Early comments are welcome.

@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch from 1a10324 to fcc87f2 Compare May 11, 2017 23:54
@pfalcon pfalcon changed the title Bootstrapping BSD Sockets like API WIP: Bootstrapping BSD Sockets like API May 12, 2017
@jukkar jukkar self-requested a review May 12, 2017 07:56
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the commit subject, I would prefer more logical naming like "net: socket: ...."
I usually try to avoid putting directory or file name into commit subject as files are moved around etc so better imho is to use logical component names here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with having a separate directory for this so we do not clutter subsys/net/ip too much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use POINTER_TO_INT(ctx) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@pfalcon
Copy link
Contributor Author

pfalcon commented May 12, 2017

For the commit subject, I would prefer more logical naming like "net: socket: ...."

Fixed, thanks.

@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch 4 times, most recently from 837e1d6 to 7512539 Compare May 12, 2017 15:25
@jukkar jukkar added the net label May 12, 2017
@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch from 7512539 to 7fd742e Compare May 13, 2017 14:23
@pfalcon pfalcon changed the title WIP: Bootstrapping BSD Sockets like API [WIP, 1.9] Bootstrapping BSD Sockets like API May 15, 2017
@nashif nashif added this to the v1.9 milestone May 15, 2017
@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch from 7fd742e to 301baee Compare June 3, 2017 17:30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example where such namespace clashes might occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Zephyr is being used together with a 3rd-party library, whose authors also doubted that namespace clashes would occur, so used names like "listen" and "close" in their API ;-).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a specific example of such 3rd-party library? Not sure how we ever have 2 providers of bsd socket APIs, What libraries that potentially will be used with Zephyr would have implemented those APIs? This is in most cases implemented by the OS, not libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example is lwIP. Another example is someone finally addressing one of the big functionality holes of Zephyr - lack of native POSIX port.

Not sure how we ever have 2 providers of bsd socket APIs, What libraries that potentially will be used with Zephyr would have implemented those APIs?

I'm not sure either, and that's good indication that best practices of namespacing should be followed, otherwise we get a situation of a crowd where everyone considers oneself exceptional and that problems of everyone else doesn't apply to them, and if they happen, it's somebody else's chore to resolve them.

This is in most cases implemented by the OS, not libraries.

Zephyr is itself a library (libzephyr.a) which real applications (consisting of many other libraries) use to get some hardware abstraction and multitasking. Maybe in a couple of decades it'll become "the OS", but standing in a big-big crowd and pretending it's the only thing isn't the smoothest way to get there ;-).

Let's again take lwIP as an example. It defines 2 related config params:

#if LWIP_COMPAT_SOCKETS
#define accept(a,b,c)         lwip_accept(a,b,c)
#define bind(a,b,c)           lwip_bind(a,b,c)
...
#if LWIP_POSIX_SOCKETS_IO_NAMES
#define read(a,b,c)           lwip_read(a,b,c)
#define write(a,b,c)          lwip_write(a,b,c)
#define close(s)              lwip_close(s)
...
#endif
#endif

As the paragraph on which you comment describes, close() alone is enough to decide - it's irresponsible to grab that name for a code which doesn't do all that normal close() does, then it should be prefixed, then everything else should be prefixed for consistency.

@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch 4 times, most recently from 6924216 to d958aaa Compare June 13, 2017 15:25
pfalcon added 10 commits June 14, 2017 01:16
This adds Kconfig and build infrastructure and implements
zsock_socket() and zsock_close() functions.

Signed-off-by: Paul Sokolovsky <[email protected]>
Two changes are required so far:

* There's unavoidable need to have a per-socket queue of packets
(for data sockets) or pending connections (for listening sockets).
These queues share the same space (as a C union).
* There's a need to track "EOF" status of connection, synchronized
with a queue of pending packets (i.e. EOF status should be processed
only when all pending packets are processed). A natural place to
store it per-packet then, and we had a "sent" bit which was used
only for outgoing packets, recast it as "eof" for incoming socket
packets.

Signed-off-by: Paul Sokolovsky <[email protected]>
With CONFIG_NET_SOCKETS_POSIX_NAMES=y, "raw" POSIX names like
socket(), recv(), close() will be exposed (using macro defines).
The close() is the biggest culprit here, because in POSIX it
applies to any file descriptor, but in this implementation -
only to sockets.

Signed-off-by: Paul Sokolovsky <[email protected]>
By moving user_data member at the beginning of structure. With
refcount at the beginning, reliable passsing of contexts via
FIFO was just impossible. (Queuing contexts to a FIFO is required
for BSD Sockets API).

Signed-off-by: Paul Sokolovsky <[email protected]>
TODO: Should really go to kernel.h.

Signed-off-by: Paul Sokolovsky <[email protected]>
Signed-off-by: Paul Sokolovsky <[email protected]>
pfalcon added 2 commits June 14, 2017 01:16
If a socket is closed without reading all data from peer or accepting
all pending connection, they will be leaked. So, flush queues
explicitly.

Signed-off-by: Paul Sokolovsky <[email protected]>
@pfalcon pfalcon force-pushed the net-sockets-bootstrap1 branch from d958aaa to 287230f Compare June 13, 2017 22:16
@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 14, 2017

Closing in favor of #498, which is resubmit of the complete patchset suitable for wide review. Hopefully, initial comments here were address and/or answered.

@pfalcon pfalcon closed this Jun 14, 2017
@zephyrbot zephyrbot mentioned this pull request Sep 23, 2017
4 tasks
@nashif nashif modified the milestones: v1.9, v1.9.0 Oct 3, 2017
nagineni pushed a commit to nagineni/zephyr that referenced this pull request Nov 20, 2017
stephanosio added a commit to stephanosio/zephyr that referenced this pull request Dec 2, 2019
NEWLIB_LIBC_NANO was defined only for gnuarmemb toolchain because
Zephyr SDK did not support the newlib nano variant (as defined by
nano.specs) due to the inherent limitations of crosstool-ng, which is
used to build the toolchains.

Since crosstool-ng/crosstool-ng#1279 added support for the nano C/C++
runtime library variant and this change was introduced to the Zephyr
SDK through zephyrproject-rtos#153, it is no longer necessary to
restrict the newlib nano variant to gnuarmemb toolchain only.

Signed-off-by: Stephanos Ioannidis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants