-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sock: Introduction of new application layer API #5758
Changes from 1 commit
7ef177a
b71c2f3
44af876
6f06def
01b0975
b24d9de
bb7736a
a4fd05c
278f046
a41bc13
1f74eae
877cad9
26ee724
777c8f7
002be06
ce205af
73861e2
d17ef91
09703e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
/* | ||
* Copyright (C) 2016 Alexander Aring <aar@pengutronix.de> | ||
* Freie Universität Berlin | ||
* HAW Hamburg | ||
* Kaspar Schleiser <kaspar@schleiser.de> | ||
* | ||
* This file is subject to the terms and conditions of the GNU Lesser | ||
* General Public License v2.1. See the file LICENSE in the top level | ||
* directory for more details. | ||
*/ | ||
|
||
/** | ||
* @defgroup net_sock Sock API | ||
* @ingroup net | ||
* @brief Provides a minimal common API for applications to connect to the | ||
* different network stacks. | ||
* | ||
* About | ||
* ===== | ||
* | ||
* +---------------+ | ||
* | Application | | ||
* +---------------+ | ||
* ^ | ||
* | | ||
* v | ||
* sock | ||
* ^ | ||
* | | ||
* v | ||
* +---------------+ | ||
* | Network Stack | | ||
* +---------------+ | ||
* | ||
* This module provides a minimal set of functions to establish connections or | ||
* send and receives datagrams using different types of communication. Together, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
"differnet types of communication"? please rephrase |
||
* they serve as a common API that connects application- and network stack code. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
* | ||
* Currently the following sock types are defined: | ||
* | ||
* * @ref sock_ip_t (net/sock/ip.h): raw IP sock | ||
* * @ref sock_tcp_t (net/sock/tcp.h): TCP sock | ||
* * @ref sock_udp_t (net/sock/udp.h): UDP sock | ||
* | ||
* Each network stack must implement at least one sock type. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can drop this. |
||
* | ||
* Note that there might be no relation between the different sock types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does that mean? |
||
* For simplicity and modularity this API doesn't put any restriction of the | ||
* actual implementation of the type. For example, one implementation might | ||
* choose to have all sock types have a common base class or use the raw IP | ||
* sock type to send e.g. UDP packets, while others will keep them | ||
* completely separate from each other. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole paragraph should go way down into a section called sth like "implementor notes". |
||
* How To Use | ||
* ========== | ||
* | ||
* A RIOT application uses the functions provided by one or more of the | ||
* sock type headers (for example @ref sock_udp), regardless of the | ||
* network stack it uses. | ||
* The network stack used under the bonnet is specified by including the | ||
* appropriate module (for example USEMODULE += gnrc_sock_udp) | ||
* | ||
* This allows for network stack agnostic code on the application layer. | ||
* The application code to establish a connection is always the same, allowing | ||
* the network stack underneath to be switched simply by changing the | ||
* `USEMODULE` definitions in the application's Makefile. | ||
* | ||
* @{ | ||
* | ||
* @file | ||
* @brief Sock API definitions | ||
* | ||
* @author Alexander Aring <aar@pengutronix.de> | ||
* @author Simon Brummer <simon.brummer@haw-hamburg.de> | ||
* @author Cenk Gündoğan <mail@cgundogan.de> | ||
* @author Peter Kietzmann <peter.kietzmann@haw-hamburg.de> | ||
* @author Martine Lenders <m.lenders@fu-berlin.de> | ||
* @author Kaspar Schleiser <kaspar@schleiser.de> | ||
*/ | ||
|
||
#ifndef NET_SOCK_H_ | ||
#define NET_SOCK_H_ | ||
|
||
#include <stdbool.h> | ||
|
||
#include "net/sock/ip.h" | ||
#include "net/sock/tcp.h" | ||
#include "net/sock/udp.h" | ||
|
||
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
#ifdef __cplusplus | ||
} | ||
#endif | ||
|
||
#endif /* NET_SOCK_H_ */ | ||
/** @} */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* Copyright (C) 2016 Freie Universität Berlin | ||
* Kaspar Schleiser <kaspar@schleiser.de> | ||
* | ||
* This file is subject to the terms and conditions of the GNU Lesser | ||
* General Public License v2.1. See the file LICENSE in the top level | ||
* directory for more details. | ||
*/ | ||
|
||
/** | ||
* @defgroup net_sock_addr Address abstractions | ||
* @ingroup net_sock | ||
* @brief Address abstractions for usage with @ref net_sock | ||
* @{ | ||
* | ||
* @file | ||
* @brief Address abstraction definitions for @ref net_sock | ||
* | ||
* @author Martine Lenders <m.lenders@fu-berlin.de> | ||
* @author Kaspar Schleiser <kaspar@schleiser.de> | ||
*/ | ||
#ifndef SOCK_ADDR_H_ | ||
#define SOCK_ADDR_H_ | ||
|
||
#include <stdint.h> | ||
|
||
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
/** | ||
* @brief Special @ref net_netif "netif" ID for "any interface" | ||
* @todo Define in @ref net_netif | ||
*/ | ||
#define SOCK_ADDR_ANY_NETIF (0) | ||
|
||
/** | ||
* @brief Address to bind to any IPv4 address | ||
*/ | ||
#define SOCK_ADDR_IPV4_ADDR_ANY (0U) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, overlooked that one in my last commit. Will do. |
||
|
||
/** | ||
* @brief Address to bind to any IPv6 address | ||
*/ | ||
#define SOCK_ADDR_IPV6_ADDR_ANY { { 0 } } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here too |
||
|
||
typedef uint32_t sock_addr_ipv4_t; | ||
|
||
/** | ||
* @brief Type to represent an IPv6 address | ||
*/ | ||
typedef struct { | ||
uint8_t u8[16]; /**< bytes of the address */ | ||
} sock_addr_ipv6_t; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need another datatype to represent an IPv6 address here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sock has it's 'own type' so the headers can be used as-is on other platforms. (I'm thinking of my POSIX implementation.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. An IPv6 address is always 16 bytes, no matter which stack or which platform. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So why is it not defined as an array of 8 uint8 everywhere? And, where We decided to include the definition here as it
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In order to deal with byte-order and provide simple access to different parts of the address through an union.
https://github.com/RIOT-OS/RIOT/blob/master/sys/include/net/ipv6/addr.h
Might be personal taste, but I find it confusing, when there are multiple data structs for the same object.
Can you elaborate? I'm only aware of the struct cited above and don't see how this should have alignment problems. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That file alone has as many lines as all the dual-stack sock headers combined, and ~200 vs ~150 SLOC. Every implementation of the API can still happily use that header. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So what does this have to with 6LoWPAN? This is just a common problem when working with unions. We could consider converting ipv6_addr_t to a simple struct, but this is a different discussion.
For me, yes, it does. One of the things that I dislike most about POSIX sockets and current
IMO adding another (redundant) data type is exactly the contrary.
I have the impression this is your main point, isn't it?
IMO it is.
198 SLOC. Your dislike of documentation does not make it invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The one aspect of it that the interface needed, the definition of
Simple and self-contained? I think our brains just work differently.
Yes I ran the tool too, this PR's sock has 152 SLOC. The point here is
Where does that come from again, and what do you mean by it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, we're running in circles here. I have (strong) dislike of defining redundant data structure, you have a dislike of the current IPv6 header. Possible resolution strategies:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your proposal in #5758 (comment) would be also a solution I could live with. |
||
|
||
/** | ||
* @brief Type to abstract both IPv4 and IPv6 addresses | ||
*/ | ||
typedef union { | ||
#ifdef SOCK_HAS_IPV6 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this ifdef if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because ipv4 is the smaller type. If a stack only supports IPv4 (or the user only wants to use IPv4) why enforce a bigger type? |
||
sock_addr_ipv6_t ipv6; /**< IPv6 address mode */ | ||
#endif | ||
sock_addr_ipv4_t ipv4; /**< IPv4 address mode */ | ||
} sock_addr_ip_t; | ||
|
||
/** | ||
* @brief Address to bind to any IPv4 address | ||
*/ | ||
static const sock_addr_ipv4_t sock_addr_ipv4_any = SOCK_ADDR_IPV4_ADDR_ANY; | ||
|
||
/** | ||
* @brief Address to bind to any IPv6 address | ||
*/ | ||
static const sock_addr_ipv6_t sock_addr_ipv6_any = SOCK_ADDR_IPV6_ADDR_ANY; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't a static inlined "memset(addr,0,16)" be more space efficient? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest… I think we can remove those. You wouldn't just null the address dynamically, but the whole end-point (compare sockaddr with sockets). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove those.
+1
|
||
|
||
#ifdef __cplusplus | ||
} | ||
#endif | ||
|
||
#endif /* SOCK_ADDR_H_ */ | ||
/** @} */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright (C) 2016 Freie Univesität Berlin | ||
* | ||
* This file is subject to the terms and conditions of the GNU Lesser | ||
* General Public License v2.1. See the file LICENSE in the top level | ||
* directory for more details. | ||
*/ | ||
|
||
/** | ||
* @defgroup net_sock_flags Common flags for @ref net_sock | ||
* @ingroup net_sock | ||
* @brief Common flags for usage with @ref net_sock | ||
* @{ | ||
* | ||
* @file | ||
* @brief Common flags definitions for usage with @ref net_sock | ||
* | ||
* @author Martine Lenders <m.lenders@fu-berlin.de> | ||
*/ | ||
#ifndef NET_SOCK_FLAGS_H_ | ||
#define NET_SOCK_FLAGS_H_ | ||
|
||
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
/** | ||
* @brief Common flags for @ref net_conn | ||
* @{ | ||
*/ | ||
#define SOCK_FLAGS_REUSE_EP (0x00000001) /**< allow to reuse end point on bind */ | ||
/** @} */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we merge this with another header? Maybe directly into sock.h? And do we really need space for so many flags? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Need to think about it, but probably you are right.
Problem with flags is always, that you not just can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMO we can drop sock.h. But as the endpoints (for tcp and udp) are
Why can we not just use unsigned? Isn't that fine until we have up 16 flags? uint8_t or uint16_t would probably involve extra masking code for 32bit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean, use unsigned and document that only the lower 16 bits can be used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Whats wrong with that? That would be for implementors only. But they can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing wrong, I was just asking if I got you right. |
||
|
||
#ifdef __cplusplus | ||
} | ||
#endif | ||
|
||
#endif /* NET_SOCK_FLAGS_H_ */ | ||
/** @} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that tagline.
The API is supposed to be used to write network applications. Would you mind to rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is so much doc change needed, why did you merge?!?