[LTP] [RFC PATCH 0/4] Add headers in lapi

[LTP] [RFC PATCH 0/4] Add headers in lapi

[Petr Vorel]

Hi,

I posted this originally in "netstress: Add imports for TCP_FASTOPEN definition" [1], got reply from Alexey [2].

Do we want to include system headers which originaly define definitions in lapi
headers? (e.g. include <linux/falloc.h> in include/lapi/fallocate.h as it
defines FALLOC_FL_KEEP_SIZE from linux/falloc.h)?
Some of them do it, some not.

If yes, do we want to check presence of them with autotools?

What is the purpose of lapi anyway? I suppose just to backport missing features
added in recent glibc to older versions or missing in other libc (musl, uclibc,
binder).


Kind regards,
Petr

[1] http://lists.linux.it/pipermail/ltp/2017-October/006275.html
[2] http://lists.linux.it/pipermail/ltp/2017-November/006345.html

Petr Vorel (4):
  lapi: Add lapi/tcp.h with definitions moved from netstress.c
  lapi/socket.h: Move definitions from netstress.c
  lapi: Add lapi/in.h with definition moved from netstress.c
  lapi: Add lapi/dccp.h with definition moved from netstress.c

 include/lapi/dccp.h                     | 25 +++++++++++++++++++++++++
 include/lapi/in.h                       | 25 +++++++++++++++++++++++++
 include/lapi/socket.h                   | 18 +++++++++++++++++-
 include/lapi/tcp.h                      | 25 +++++++++++++++++++++++++
 testcases/network/netstress/netstress.c | 33 ++++-----------------------------
 5 files changed, 96 insertions(+), 30 deletions(-)
 create mode 100644 include/lapi/dccp.h
 create mode 100644 include/lapi/in.h
 create mode 100644 include/lapi/tcp.h

-- 
2.15.0

[LTP] [RFC PATCH 1/4] lapi: Add lapi/tcp.h with definitions moved from netstress.c

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/tcp.h                      | 25 +++++++++++++++++++++++++
 testcases/network/netstress/netstress.c |  6 +-----
 2 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100644 include/lapi/tcp.h

diff --git a/include/lapi/tcp.h b/include/lapi/tcp.h
new file mode 100644
index 000000000..44d75ab7b
--- /dev/null
+++ b/include/lapi/tcp.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2017 Petr Vorel <pvorel@suse.cz>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef TCP_H__
+#define TCP_H__
+
+#ifndef TCP_FASTOPEN
+#define TCP_FASTOPEN	23
+#endif
+
+#endif	/* TCP_H__ */
diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index 4b211aa75..d45c858dc 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -33,6 +33,7 @@
 #include <errno.h>
 
 #include "lapi/posix_clocks.h"
+#include "lapi/tcp.h"
 #include "tst_safe_pthread.h"
 #include "tst_test.h"
 
@@ -51,11 +52,6 @@ static const int max_msg_len = (1 << 16) - 1;
 #define DCCP_SOCKOPT_SERVICE	2
 #endif
 
-/* TCP server requiers */
-#ifndef TCP_FASTOPEN
-#define TCP_FASTOPEN	23
-#endif
-
 #ifndef SO_BUSY_POLL
 #define SO_BUSY_POLL	46
 #endif
-- 
2.15.0

[LTP] [RFC PATCH 2/4] lapi/socket.h: Move definitions from netstress.c

[Petr Vorel]

+ remove sys/socket.h from netstress.c

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/socket.h                   | 18 +++++++++++++++++-
 testcases/network/netstress/netstress.c | 18 ++----------------
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/include/lapi/socket.h b/include/lapi/socket.h
index 98263105e..af9448804 100644
--- a/include/lapi/socket.h
+++ b/include/lapi/socket.h
@@ -19,8 +19,24 @@
 #ifndef __LAPI_SOCKET_H__
 #define __LAPI_SOCKET_H__
 
+#ifndef MSG_FASTOPEN
+#define MSG_FASTOPEN	0x20000000 /* Send data in TCP SYN */
+#endif
+
+#ifndef SO_BUSY_POLL
+#define SO_BUSY_POLL	46
+#endif
+
 #ifndef SOCK_CLOEXEC
-# define SOCK_CLOEXEC 02000000
+#define SOCK_CLOEXEC 02000000
+#endif
+
+#ifndef SOCK_DCCP
+#define SOCK_DCCP		6
+#endif
+
+#ifndef SOL_DCCP
+#define SOL_DCCP		269
 #endif
 
 #endif /* __LAPI_SOCKET_H__ */
diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index d45c858dc..790a76e20 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -22,7 +22,6 @@
 #include <stdlib.h>
 #include <limits.h>
 #include <sys/types.h>
-#include <sys/socket.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -33,34 +32,21 @@
 #include <errno.h>
 
 #include "lapi/posix_clocks.h"
+#include "lapi/socket.h"
 #include "lapi/tcp.h"
 #include "tst_safe_pthread.h"
 #include "tst_test.h"
 
 static const int max_msg_len = (1 << 16) - 1;
 
-#ifndef SOCK_DCCP
-#define SOCK_DCCP		6
-#endif
 #ifndef IPPROTO_DCCP
 #define IPPROTO_DCCP		33
 #endif
-#ifndef SOL_DCCP
-#define SOL_DCCP		269
-#endif
+
 #ifndef DCCP_SOCKOPT_SERVICE
 #define DCCP_SOCKOPT_SERVICE	2
 #endif
 
-#ifndef SO_BUSY_POLL
-#define SO_BUSY_POLL	46
-#endif
-
-/* TCP client requiers */
-#ifndef MSG_FASTOPEN
-#define MSG_FASTOPEN	0x20000000 /* Send data in TCP SYN */
-#endif
-
 enum {
 	SERVER_HOST = 0,
 	CLIENT_HOST,
-- 
2.15.0

[LTP] [RFC PATCH 3/4] lapi: Add lapi/in.h with definition moved from netstress.c

[Petr Vorel]

+ remove now unused include netinet/in.h

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/in.h                       | 25 +++++++++++++++++++++++++
 testcases/network/netstress/netstress.c |  6 +-----
 2 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100644 include/lapi/in.h

diff --git a/include/lapi/in.h b/include/lapi/in.h
new file mode 100644
index 000000000..9e623fa61
--- /dev/null
+++ b/include/lapi/in.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2017 Petr Vorel <pvorel@suse.cz>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef IN_H__
+#define IN_H__
+
+#ifndef IPPROTO_DCCP
+#define IPPROTO_DCCP		33
+#endif
+
+#endif	/* IN_H__ */
diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index 790a76e20..8a9736a71 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -23,7 +23,6 @@
 #include <limits.h>
 #include <sys/types.h>
 #include <netdb.h>
-#include <netinet/in.h>
 #include <arpa/inet.h>
 #include <poll.h>
 #include <time.h>
@@ -31,6 +30,7 @@
 #include <unistd.h>
 #include <errno.h>
 
+#include "lapi/in.h"
 #include "lapi/posix_clocks.h"
 #include "lapi/socket.h"
 #include "lapi/tcp.h"
@@ -39,10 +39,6 @@
 
 static const int max_msg_len = (1 << 16) - 1;
 
-#ifndef IPPROTO_DCCP
-#define IPPROTO_DCCP		33
-#endif
-
 #ifndef DCCP_SOCKOPT_SERVICE
 #define DCCP_SOCKOPT_SERVICE	2
 #endif
-- 
2.15.0

[LTP] [RFC PATCH 4/4] lapi: Add lapi/dccp.h with definition moved from netstress.c

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/dccp.h                     | 25 +++++++++++++++++++++++++
 testcases/network/netstress/netstress.c |  5 +----
 2 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 include/lapi/dccp.h

diff --git a/include/lapi/dccp.h b/include/lapi/dccp.h
new file mode 100644
index 000000000..e4b19f240
--- /dev/null
+++ b/include/lapi/dccp.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2017 Petr Vorel <pvorel@suse.cz>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef DCCP_H__
+#define DCCP_H__
+
+#ifndef DCCP_SOCKOPT_SERVICE
+#define DCCP_SOCKOPT_SERVICE	2
+#endif
+
+#endif	/* DCCP_H__ */
diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index 8a9736a71..53a43fbef 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -30,6 +30,7 @@
 #include <unistd.h>
 #include <errno.h>
 
+#include "lapi/dccp.h"
 #include "lapi/in.h"
 #include "lapi/posix_clocks.h"
 #include "lapi/socket.h"
@@ -39,10 +40,6 @@
 
 static const int max_msg_len = (1 << 16) - 1;
 
-#ifndef DCCP_SOCKOPT_SERVICE
-#define DCCP_SOCKOPT_SERVICE	2
-#endif
-
 enum {
 	SERVER_HOST = 0,
 	CLIENT_HOST,
-- 
2.15.0

[LTP] [RFC PATCH 0/4] Add headers in lapi

[Cyril Hrubis]

Hi!
> I posted this originally in "netstress: Add imports for TCP_FASTOPEN definition" [1], got reply from Alexey [2].
> 
> Do we want to include system headers which originaly define definitions in lapi
> headers? (e.g. include <linux/falloc.h> in include/lapi/fallocate.h as it
> defines FALLOC_FL_KEEP_SIZE from linux/falloc.h)?
> Some of them do it, some not.

I think that it's a bit cleaner to include the system header before we
define the fallback definitions, since that way we are sure to pick the
system ones even if test writer forgets to include the system header in
the test.

But be wary of combining the linux and glibc headers, sometimes that
causes collisions and all kinds of problems.

In the case of fallocate the bits/fcntl-linux.h which is included by
bits/fcntl.h which is included from fcntl.h defines these constants (on
older glibcs, newer include linux/fallocate.h in bits/fcntl-linux.h), so
including both fcntl.h and linux/falloc.h will cause macro
redefinitions in certain cases...

> If yes, do we want to check presence of them with autotools?

Most of the time we do not have to, since these headers are present
everywhere. I do not think that we should be adding autoconf checks just
in case. So until there is a breakage we should assume that check is not
needed.

> What is the purpose of lapi anyway? I suppose just to backport missing features
> added in recent glibc to older versions or missing in other libc (musl, uclibc,
> binder).

The purpose is to have fallback definitions in a single place,
previously these were repeated over and over in each testcase, and
expecially with syscall wrappers that sometimes differ sublty between
architectures this was quite complicated to maintain.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 2/4] lapi/socket.h: Move definitions from netstress.c

[Cyril Hrubis]

Hi!
>  #ifndef SOCK_CLOEXEC
> -# define SOCK_CLOEXEC 02000000
> +#define SOCK_CLOEXEC 02000000
> +#endif

FYI the prefered way how to indent innner macros are spaces after the
hash #.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 3/4] lapi: Add lapi/in.h with definition moved from netstress.c

[Cyril Hrubis]

> --- /dev/null
> +++ b/include/lapi/in.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (c) 2017 Petr Vorel <pvorel@suse.cz>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef IN_H__
> +#define IN_H__
> +
> +#ifndef IPPROTO_DCCP
> +#define IPPROTO_DCCP		33
> +#endif
> +
> +#endif	/* IN_H__ */

Maybe we should rename this file to netinet_in.h or create netinet
directory in lapi. Including just lapi/in.h is a bit confusing.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 0/4] Add headers in lapi

[Petr Vorel]

Hi Cyril,

thanks for info!

> I think that it's a bit cleaner to include the system header before we
> define the fallback definitions, since that way we are sure to pick the
> system ones even if test writer forgets to include the system header in
> the test.
OK. And so it's proposed to include system headers even when using lapi headers.
This looks to me as a duplicity, but probably good practise.

> But be wary of combining the linux and glibc headers, sometimes that
> causes collisions and all kinds of problems.

> In the case of fallocate the bits/fcntl-linux.h which is included by
> bits/fcntl.h which is included from fcntl.h defines these constants (on
> older glibcs, newer include linux/fallocate.h in bits/fcntl-linux.h), so
> including both fcntl.h and linux/falloc.h will cause macro
> redefinitions in certain cases...
OK. IMHO most of the time glibc headers are enough (it's a question what will be newer in
old distro: glibc header or kernel one).

> > If yes, do we want to check presence of them with autotools?

> Most of the time we do not have to, since these headers are present
> everywhere. I do not think that we should be adding autoconf checks just
> in case. So until there is a breakage we should assume that check is not
> needed.
Right!

> > What is the purpose of lapi anyway? I suppose just to backport missing features
> > added in recent glibc to older versions or missing in other libc (musl, uclibc,
> > binder).

> The purpose is to have fallback definitions in a single place,
> previously these were repeated over and over in each testcase, and
> expecially with syscall wrappers that sometimes differ sublty between
> architectures this was quite complicated to maintain.
Sure, I understand DRY principle. I just wasn't sure of the case in
testcases/network/netstress/netstress.c where no system header was used (none of
linux/tcp.h and netinet/tcp.h) and macro was defined, but missing header was probably
left accidentally. Checking that TCP_FASTOPEN was added in 2.18, so it's not in older
distros and in current RHEL (so it's valid to be in lapi header).


Kind regards,
Petr

[LTP] [RFC PATCH 3/4] lapi: Add lapi/in.h with definition moved from netstress.c

[Petr Vorel]

> Maybe we should rename this file to netinet_in.h or create netinet
> directory in lapi. Including just lapi/in.h is a bit confusing.
Is it because in.h is also in bits/ directory (not only in linux and some glibc directory)?
/usr/include/bits/in.h
/usr/include/netinet/in.h
/usr/include/linux/in.h

As the same we have with lapi/tcp.h (in both kernel and glibc)...
/usr/include/netinet/tcp.h
/usr/include/linux/tcp.h

And I'm for directory.

Kind regards,
Petr

[LTP] [RFC PATCH 2/4] lapi/socket.h: Move definitions from netstress.c

[Alexey Kodanev]

On 11/09/2017 02:39 PM, Petr Vorel wrote:
> + remove sys/socket.h from netstress.c

What about including it in lapi/socket.h?

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  include/lapi/socket.h                   | 18 +++++++++++++++++-
>  testcases/network/netstress/netstress.c | 18 ++----------------
>  2 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/include/lapi/socket.h b/include/lapi/socket.h
> index 98263105e..af9448804 100644
> --- a/include/lapi/socket.h
> +++ b/include/lapi/socket.h
> @@ -19,8 +19,24 @@
>  #ifndef __LAPI_SOCKET_H__
>  #define __LAPI_SOCKET_H__
>  
> +#ifndef MSG_FASTOPEN
> +#define MSG_FASTOPEN	0x20000000 /* Send data in TCP SYN */
> +#endif
> +
> +#ifndef SO_BUSY_POLL
> +#define SO_BUSY_POLL	46
> +#endif
> +
>  #ifndef SOCK_CLOEXEC
> -# define SOCK_CLOEXEC 02000000
> +#define SOCK_CLOEXEC 02000000
> +#endif
> +
> +#ifndef SOCK_DCCP
> +#define SOCK_DCCP		6
> +#endif

The two above should probably be in ascending order...

SOCK_CLOEXEC along with SOCK_NONBLOCK already defined in
lapi/fcntl.h. I guess we should move them to lapi/socket.h instead.

Thanks,
Alexey

[LTP] [RFC PATCH 2/4] lapi/socket.h: Move definitions from netstress.c

[Petr Vorel]

> On 11/09/2017 02:39 PM, Petr Vorel wrote:
> > + remove sys/socket.h from netstress.c

> What about including it in lapi/socket.h?

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  include/lapi/socket.h                   | 18 +++++++++++++++++-
> >  testcases/network/netstress/netstress.c | 18 ++----------------
> >  2 files changed, 19 insertions(+), 17 deletions(-)

> > diff --git a/include/lapi/socket.h b/include/lapi/socket.h
> > index 98263105e..af9448804 100644
> > --- a/include/lapi/socket.h
> > +++ b/include/lapi/socket.h
> > @@ -19,8 +19,24 @@
> >  #ifndef __LAPI_SOCKET_H__
> >  #define __LAPI_SOCKET_H__

> > +#ifndef MSG_FASTOPEN
> > +#define MSG_FASTOPEN	0x20000000 /* Send data in TCP SYN */
> > +#endif
> > +
> > +#ifndef SO_BUSY_POLL
> > +#define SO_BUSY_POLL	46
> > +#endif
> > +
> >  #ifndef SOCK_CLOEXEC
> > -# define SOCK_CLOEXEC 02000000
> > +#define SOCK_CLOEXEC 02000000
> > +#endif
> > +
> > +#ifndef SOCK_DCCP
> > +#define SOCK_DCCP		6
> > +#endif

> The two above should probably be in ascending order...
Thanks, I'll fix it.

> SOCK_CLOEXEC along with SOCK_NONBLOCK already defined in
> lapi/fcntl.h. I guess we should move them to lapi/socket.h instead.
Good point. include/lapi/fcntl.h defines:
#ifndef O_CLOEXEC
# define O_CLOEXEC 02000000
#endif

#ifndef SOCK_CLOEXEC
# define SOCK_CLOEXEC O_CLOEXEC
#endif

This actually makes sense as these both have the same value on the same architecture, but
in glibc headers are these defined separately (in /usr/include/bits/socket_type.h and
/usr/include/asm-generic/fcntl.h).
Both musl and uclibc-ng use the same approach, so I propose to split these definitions in
according to (g)libc headers.


Kind regards,
Petr

[LTP] [RFC PATCH 3/4] lapi: Add lapi/in.h with definition moved from netstress.c

[Petr Vorel]

Hi,

> > Maybe we should rename this file to netinet_in.h or create netinet
> > directory in lapi. Including just lapi/in.h is a bit confusing.
> Is it because in.h is also in bits/ directory (not only in linux and some glibc directory)?
> /usr/include/bits/in.h
> /usr/include/netinet/in.h
> /usr/include/linux/in.h

> As the same we have with lapi/tcp.h (in both kernel and glibc)...
> /usr/include/netinet/tcp.h
> /usr/include/linux/tcp.h

> And I'm for directory.
Actually I'd be for just plain "in.h" as other fileas in lapi/ directory don't prefix
anything, aren't in any special directory.
But if you really don't like that, I'll put it into netinet_in.h.


Kind regards,
Petr