Skip to content
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

Merged
merged 19 commits into from
Sep 27, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7ef177a
sock: Introduction of new application layer API
miri64 Jun 4, 2016
b71c2f3
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
44af876
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
6f06def
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
01b0975
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
b24d9de
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
bb7736a
fixup! sock: Introduction of new application layer API
miri64 Aug 19, 2016
a4fd05c
fixup! sock: Introduction of new application layer API
miri64 Aug 20, 2016
278f046
fixup! sock: Introduction of new application layer API
miri64 Aug 20, 2016
a41bc13
fixup! sock: Introduction of new application layer API
miri64 Aug 20, 2016
1f74eae
fixup! sock: Introduction of new application layer API
miri64 Aug 22, 2016
877cad9
fixup! sock: Introduction of new application layer API
miri64 Aug 22, 2016
26ee724
fixup! sock: Introduction of new application layer API
miri64 Aug 22, 2016
777c8f7
fixup! sock: Introduction of new application layer API
miri64 Aug 24, 2016
002be06
fixup! sock: Introduction of new application layer API
miri64 Aug 25, 2016
ce205af
fixup! sock: Introduction of new application layer API
miri64 Aug 25, 2016
73861e2
fixup! sock: Introduction of new application layer API
miri64 Aug 25, 2016
d17ef91
fixup! sock: Introduction of new application layer API
miri64 Sep 2, 2016
09703e1
fixup! sock: Introduction of new application layer API
miri64 Sep 2, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions sys/include/net/sock.h
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
Copy link
Contributor

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?

Copy link
Member Author

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?!?

* 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • why minimal set? (pls drop if you can't explain)
  • receives -> receive

"differnet types of communication"? please rephrase

* they serve as a common API that connects application- and network stack code.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • why common?
  • again, I find it misleading to describe this API as "connecting application with network stack code".

*
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
*
Copy link
Contributor

Choose a reason for hiding this comment

The 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_ */
/** @} */
81 changes: 81 additions & 0 deletions sys/include/net/sock/addr.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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe SOCK_ADDR_IPV4_ANY (one less ADDR)?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 } }
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need another datatype to represent an IPv6 address here?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.)

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

So why is it not defined as an array of 8 uint8 everywhere? And, where
exactly is it defined, on all those stacks and platforms?

We decided to include the definition here as it

  1. doesn't matter much if it is again defined here, but it keeps the API
    definition complete
  2. implementors don't have to deal with it
  3. RIOT's existing definitions where scattered and loaded with possible
    alignment problems

Copy link
Member

Choose a reason for hiding this comment

The 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?

In order to deal with byte-order and provide simple access to different parts of the address through an union.

And, where exactly is it defined, on all those stacks and platforms?

https://github.com/RIOT-OS/RIOT/blob/master/sys/include/net/ipv6/addr.h

  1. doesn't matter much if it is again defined here, but it keeps the API definition complete
  2. implementors don't have to deal with it

Might be personal taste, but I find it confusing, when there are multiple data structs for the same object.

RIOT's existing definitions where scattered and loaded with possible alignment problems

Can you elaborate? I'm only aware of the struct cited above and don't see how this should have alignment problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

RIOT's
ipv6/addr.h is not exactly that.

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.

Copy link
Member

@OlegHahm OlegHahm Aug 18, 2016

Choose a reason for hiding this comment

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

You cut that out: "If that address gets casted to RIOT's ipv6_addr_t and then accessed using any of the non-uint8_t union members, that access is unaligned."

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.

Does it matter which type gets used in the API?

For me, yes, it does. One of the things that I dislike most about POSIX sockets and current conn are many additional data types.

We tried to keep the interface simple

IMO adding another (redundant) data type is exactly the contrary.

and self-contained.

I have the impression this is your main point, isn't it?

RIOT's ipv6/addr.h is not exactly that.

IMO it is.

That file alone has as many lines as all the dual-stack sock headers combined, and ~200 vs ~150 SLOC.

sloc sys/include/net/ipv6/addr.h
  Language  Files  Code  Comment  Blank  Total
     Total      1   198      468    131    786

198 SLOC.

Your dislike of documentation does not make it invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

You cut that out: "If that address gets casted to RIOT's ipv6_addr_t and then accessed using any of the non-uint8_t union members, that access is unaligned."

So what dos 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.

The one aspect of it that the interface needed, the definition of
uint8_t[16] to a name, is done in a problematic way. I just mentioned
6lowpan because that was the first thing I could think of where the
unaligned access might happen by accident.

RIOT's ipv6/addr.h is not exactly that.

IMO it is.

Simple and self-contained? I think our brains just work differently.

That file alone has as many lines as all the dual-stack sock headers combined, and ~200 vs ~150 SLOC.

sloc sys/include/net/ipv6/addr.h
  Language  Files  Code  Comment  Blank  Total
     Total      1   198      468    131    786

198 SLOC.

Yes I ran the tool too, this PR's sock has 152 SLOC. The point here is
that only three of the 198 lines are needed for the interface, and
already part of the 152.

Your dislike of documentation does not make it invalid.

Where does that come from again, and what do you mean by it?

Copy link
Member

Choose a reason for hiding this comment

The 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:

  1. I live with yet another IP address data type,
  2. you live with including net/ipv6 here,
  3. we define a data type for IPv6 addresses somewhere else, similar to the proposal here and replace both data structures.

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

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

Why this ifdef if sock_addr_ipv6_t is definted a few lines above? Why no ifdef for ipv4?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

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


#ifdef __cplusplus
}
#endif

#endif /* SOCK_ADDR_H_ */
/** @} */
39 changes: 39 additions & 0 deletions sys/include/net/sock/flags.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 */
/** @} */
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Need to think about it, but probably you are right.

And do we really need space for so many flags?

Problem with flags is always, that you not just can use unsigned (otherwise I would prefer it, since the compiler optimizes it). But true, on non 32-platforms the code size would grow anyway, so maybe a non-32-bit value is better. For now it is the only flag, since that was the only option that came to our mind during the meeting, so uint8_t might suffice (in case other flags come to mind later). Or shall we go for uin16_t just in case?

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Need to think about it, but probably you are right.

IMO we can drop sock.h. But as the endpoints (for tcp and udp) are
identical, maybe they can move to a shared ep.h?

And do we really need space for so many flags?

Problem with flags is always, that you not just can use unsigned (otherwise I would prefer it, since the compiler optimizes it). But true, on non 32-platforms the code size would grow anyway, so maybe a non-32-bit value is better. For now it is the only flag, since that was the only option that came to our mind during the meeting, so uint8_t might suffice (in case other flags come to mind later). Or shall we go for uin16_t just in case?

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
platforms.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Whats wrong with that? That would be for implementors only. But they can
even decide to assert on wrong usage.

Copy link
Member

Choose a reason for hiding this comment

The 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_ */
/** @} */
Loading