Re: [PATCH] time.h: include header before using time_t

[Kurt Van Dijck]

On vr, 04 okt 2019 13:52:11 -0400, James Carlson wrote:
> On 10/04/19 13:40, Kurt Van Dijck wrote:
> > I think you confirm 4x what I said, but I probably expressed myself
> > badly, so "show me code!", I created this patch.
> > It (1) works for me and (2) does not mix userspace headers in kernel
> > space anywhere.
> > Would this work for you?
> 
> That seems ok, in as much as it compiles on Solaris.  But I'm still a
> little confused about your apparent opposition to  at the
> point where time_t is actually used.
> 
>  is part of the UNIX standards.  It's documented to define
> time_t (among other things).  It's on-point for a header file that may
> be used in kernel context.  What's the concern?

headers under sys/ are, AFAIK, not delivered by the kernel, but by the
toolchain. sys/time.h may have less issues than time.h, it has the same
disease.

But maybe I'm incompetent on the matter, my knowledge besides linux on
this matter is very limited.

Kurt

[PATCH] time.h: include header before using time_t

[Kurt Van Dijck]

On vr, 04 okt 2019 10:49:17 -0400, James Carlson wrote:
> On 10/04/19 10:29, Kurt Van Dijck wrote:
> > Now that I know that that file is used as include for kernel code, I'd
> > rather include time.h in the userspace c-files.
> 
> My point is that include/net/ isn't strictly userspace.
> 
> If you feel the need, then go ahead and include  in user level
> files.  This just isn't one of those.
> 
> If you must do this in ppp_def.h, then it needs to be guarded against
> *all* of the systems where including a top-level header file inside a
> kernel module is the wrong thing to do, not just "ifndef SOLARIS".  Do
> you know which systems those are?  I can tell you that Solaris/Illumos
> is at least one such system, but I can't tell you that it's *all* of them.
> 
> I think this include is out of place here.
ack

I think you confirm 4x what I said, but I probably expressed myself
badly, so "show me code!", I created this patch.
It (1) works for me and (2) does not mix userspace headers in kernel
space anywhere.
Would this work for you?

---
commit 567d505b1b8eff3d1579e849a4272d114f047bf3
Author: Kurt Van Dijck 
Date:   Fri Oct 4 19:24:22 2019

time.h: include header before using time_t

Since include/net/ppp_defs.h is used in both kernelspace and userland
makes it hard to put time.h include there.
This commit fixes the problems in userspace code individually and leaves
ppp_defs.h as-is.

Signed-off-by: Kurt Van Dijck 

diff --git a/pppd/plugins/rp-pppoe/pppoe-discovery.c 
b/pppd/plugins/rp-pppoe/pppoe-discovery.c
index 8b2e946..f19c6d8 100644
--- a/pppd/plugins/rp-pppoe/pppoe-discovery.c
+++ b/pppd/plugins/rp-pppoe/pppoe-discovery.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "pppoe.h"
 
diff --git a/pppd/sha1.c b/pppd/sha1.c
index f4f975c..4e51cee 100644
--- a/pppd/sha1.c
+++ b/pppd/sha1.c
@@ -17,6 +17,7 @@
 /* #define SHA1HANDSOFF * Copies data before messing with it. */
 
 #include 
+#include 
 #include /* htonl() */
 #include 
 #include "sha1.h"

Re: [PATCH 4/9] pppd: include time.h before using time_t

[Kurt Van Dijck]

On vr, 04 okt 2019 08:52:12 -0400, James Carlson wrote:
> On 10/04/19 06:49, Kurt Van Dijck wrote:
> >> IMHO time_t is defined in sys/types.h
> > 
> > http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
> > chapter 7.23.1.3
> > 
> 
> I believe that covers userland environments, not the kernel.
> 
> At least on Solaris (and its derivatives, such as Illumos), the symbols
> available in the kernel are defined in sys/ (or net/, netinet/, or
> similar for network bits).  The top-level header files are for userland
> libraries.  Userland libraries are not accessible within the kernel.
> 
> In this case, the common net/ppp_defs.h file is used by both user-level
> code (pppd itself) and by several kernel modules.

I see.

> 
> There may be systems on which including  within a kernel module
> is harmless (I suspect Linux is one), but I have a hard time believing
> that it's correct to do so.

You're right that the kernel code does not __necessarily__ use the same thing.
What matters here is that all kernel code must use the same thing.

> 
> Do you know of a system where either (a)  does not exist or
> (b) it exists but does not define 'time_t'?  I haven't been able to find
> a system that matches either case.  I tried several flavors of Linux,
> AIX, Solaris, HP/UX, and IBM USS on z/OS.

I don't know a system where (a) or (b) are valid. My point is that such
system could could exist, so I learned not to inspect the header files
looking for a type, but inspect man-pages or specifications when looking
for a type, and so time_t is defined in time.h.

Regardless of those systems, you look for 1 header that suits userspace
and solaris kernel. Isn't that a bit ... strange.

Now that I know that that file is used as include for kernel code, I'd
rather include time.h in the userspace c-files.

Kurt

Re: [PATCH 4/9] pppd: include time.h before using time_t

[Kurt Van Dijck]

> IMHO time_t is defined in sys/types.h

http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
chapter 7.23.1.3

[PATCH 3/9 v2] radius: make rc_own_bind_ipaddress available through radiusclient.h

[Kurt Van Dijck]

This commit adds a missing function declaration to the header file and
removes the compiler warning.

Signed-off-by: Kurt Van Dijck 
---
 pppd/plugins/radius/radiusclient.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pppd/plugins/radius/radiusclient.h 
b/pppd/plugins/radius/radiusclient.h
index 51b959a..cff0c26 100644
--- a/pppd/plugins/radius/radiusclient.h
+++ b/pppd/plugins/radius/radiusclient.h
@@ -440,6 +440,7 @@ UINT4 rc_get_ipaddr __P((char *));
 int rc_good_ipaddr __P((char *));
 const char *rc_ip_hostname __P((UINT4));
 UINT4 rc_own_ipaddress __P((void));
+UINT4 rc_own_bind_ipaddress __P((void));
 
 
 /* sendserver.c*/

Re: [PATCH 1/9] magic: remove K style of arguments

[Kurt Van Dijck]

On vr, 27 sep 2019 11:36:11 +1000, Paul Mackerras wrote:
> On Thu, Sep 26, 2019 at 09:20:58AM +0200, Kurt Van Dijck wrote:
> > The __P() macro does not exist in libmusl e.g.
> > I swicthed magic.{c,h} to using the std-c argument style, which had
> > already been used in some functions.
> 
> Right.  I was thinking of doing this across the whole tree in fact.

-Wmissing-parameter-type will get you through the job :-)
Until then, this patch in magic.{h,c} helps me getting things done using
musl.

Kurt

[PATCH 8/9] make: avoid using host include for cross-compiling

[Kurt Van Dijck]

Prepend include paths with the toolchain's sysroot directory.
In case of a non-sysroot-aware toolchain, this does not help,
but does not break either.

Signed-off-by: Kurt Van Dijck 
---
 pppd/Makefile.linux | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pppd/Makefile.linux b/pppd/Makefile.linux
index 8d5ce99..12a986e 100644
--- a/pppd/Makefile.linux
+++ b/pppd/Makefile.linux
@@ -125,7 +125,7 @@ CFLAGS   += -DHAS_SHADOW
 #LIBS += -lshadow $(LIBS)
 endif
 
-ifneq ($(wildcard /usr/include/crypt.h),)
+ifneq ($(wildcard $(shell $CC --print-sysroot)/usr/include/crypt.h),)
 CFLAGS  += -DHAVE_CRYPT_H=1
 LIBS   += -lcrypt
 endif
@@ -137,7 +137,7 @@ endif
 
 ifdef NEEDDES
 ifndef USE_CRYPT
-CFLAGS   += -I/usr/include/openssl
+CFLAGS   += -I$(shell $CC --print-sysroot)/usr/include/openssl
 LIBS += -lcrypto
 else
 CFLAGS   += -DUSE_CRYPT=1
-- 
1.8.5.rc3

[PATCH 1/9] magic: remove K style of arguments

[Kurt Van Dijck]

The __P() macro does not exist in libmusl e.g.
I swicthed magic.{c,h} to using the std-c argument style, which had
already been used in some functions.

Signed-off-by: Kurt Van Dijck 
---
 pppd/magic.c | 15 +++
 pppd/magic.h |  6 +++---
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/pppd/magic.c b/pppd/magic.c
index 2fb23ff..3f3ce32 100644
--- a/pppd/magic.c
+++ b/pppd/magic.c
@@ -53,8 +53,8 @@
 
 static const char rcsid[] = RCSID;
 
-extern long mrand48 __P((void));
-extern void srand48 __P((long));
+extern long mrand48 (void);
+extern void srand48 (long);
 
 /*
  * magic_init - Initialize the magic number generator.
@@ -64,7 +64,7 @@ extern void srand48 __P((long));
  * and current time, currently.
  */
 void
-magic_init()
+magic_init(void)
 {
 long seed;
 struct timeval t;
@@ -78,7 +78,7 @@ magic_init()
  * magic - Returns the next magic number.
  */
 u_int32_t
-magic()
+magic(void)
 {
 return (u_int32_t) mrand48();
 }
@@ -102,20 +102,19 @@ random_bytes(unsigned char *buf, int len)
  */
 
 double
-drand48()
+drand48(void)
 {
 return (double)random() / (double)0x7fffL; /* 2**31-1 */
 }
 
 long
-mrand48()
+mrand48(void)
 {
 return random();
 }
 
 void
-srand48(seedval)
-long seedval;
+srand48(long seedval)
 {
 srandom((int)seedval);
 }
diff --git a/pppd/magic.h b/pppd/magic.h
index c81213b..9d399e3 100644
--- a/pppd/magic.h
+++ b/pppd/magic.h
@@ -42,8 +42,8 @@
  * $Id: magic.h,v 1.5 2003/06/11 23:56:26 paulus Exp $
  */
 
-void magic_init __P((void));   /* Initialize the magic number generator */
-u_int32_t magic __P((void));   /* Returns the next magic number */
+void magic_init (void);/* Initialize the magic number generator */
+u_int32_t magic (void);/* Returns the next magic number */
 
 /* Fill buffer with random bytes */
-void random_bytes __P((unsigned char *buf, int len));
+void random_bytes (unsigned char *buf, int len);
-- 
1.8.5.rc3

[PATCH 3/9] radius: make rc_own_bind_ipaddress available through radiusclient.h

[Kurt Van Dijck]

Signed-off-by: Kurt Van Dijck 
---
 pppd/plugins/radius/radiusclient.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pppd/plugins/radius/radiusclient.h 
b/pppd/plugins/radius/radiusclient.h
index 51b959a..cff0c26 100644
--- a/pppd/plugins/radius/radiusclient.h
+++ b/pppd/plugins/radius/radiusclient.h
@@ -440,6 +440,7 @@ UINT4 rc_get_ipaddr __P((char *));
 int rc_good_ipaddr __P((char *));
 const char *rc_ip_hostname __P((UINT4));
 UINT4 rc_own_ipaddress __P((void));
+UINT4 rc_own_bind_ipaddress __P((void));
 
 
 /* sendserver.c*/
-- 
1.8.5.rc3

[PATCH 6/9] pppd: remove unused rcsid variables

[Kurt Van Dijck]

Signed-off-by: Kurt Van Dijck 
---
 pppd/auth.c| 1 -
 pppd/cbcp.c| 1 -
 pppd/ccp.c | 1 -
 pppd/chap_ms.c | 1 -
 pppd/demand.c  | 1 -
 pppd/eap.c | 1 -
 pppd/ecp.c | 1 -
 pppd/eui64.c   | 1 -
 pppd/fsm.c | 1 -
 pppd/ipcp.c| 1 -
 pppd/ipv6cp.c  | 1 -
 pppd/ipxcp.c   | 1 -
 pppd/lcp.c | 1 -
 pppd/magic.c   | 1 -
 pppd/main.c| 1 -
 pppd/options.c | 1 -
 pppd/sys-solaris.c | 1 -
 pppd/upap.c| 1 -
 pppd/utils.c   | 1 -
 19 files changed, 19 deletions(-)

diff --git a/pppd/auth.c b/pppd/auth.c
index 7457eda..f5c9acd 100644
--- a/pppd/auth.c
+++ b/pppd/auth.c
@@ -119,7 +119,6 @@
 #include "pathnames.h"
 #include "session.h"
 
-static const char rcsid[] = RCSID;
 
 /* Bits in scan_authfile return value */
 #define NONWILD_SERVER 1
diff --git a/pppd/cbcp.c b/pppd/cbcp.c
index 7f2f787..f735ab9 100644
--- a/pppd/cbcp.c
+++ b/pppd/cbcp.c
@@ -45,7 +45,6 @@
 #include "fsm.h"
 #include "lcp.h"
 
-static const char rcsid[] = RCSID;
 
 /*
  * Options.
diff --git a/pppd/ccp.c b/pppd/ccp.c
index 7d7922a..61947d9 100644
--- a/pppd/ccp.c
+++ b/pppd/ccp.c
@@ -43,7 +43,6 @@
 #include "lcp.h"   /* lcp_close(), lcp_fsm */
 #endif
 
-static const char rcsid[] = RCSID;
 
 /*
  * Unfortunately there is a bug in zlib which means that using a
diff --git a/pppd/chap_ms.c b/pppd/chap_ms.c
index c2bd00f..1de5042 100644
--- a/pppd/chap_ms.c
+++ b/pppd/chap_ms.c
@@ -94,7 +94,6 @@
 #include "pppcrypt.h"
 #include "magic.h"
 
-static const char rcsid[] = RCSID;
 
 
 static voidascii2unicode __P((char[], int, u_char[]));
diff --git a/pppd/demand.c b/pppd/demand.c
index 5e57658..72e379c 100644
--- a/pppd/demand.c
+++ b/pppd/demand.c
@@ -52,7 +52,6 @@
 #include "ipcp.h"
 #include "lcp.h"
 
-static const char rcsid[] = RCSID;
 
 char *frame;
 int framelen;
diff --git a/pppd/eap.c b/pppd/eap.c
index 6ea6c1f..94407f5 100644
--- a/pppd/eap.c
+++ b/pppd/eap.c
@@ -76,7 +76,6 @@
 #defineSHA_DIGESTSIZE 20
 #endif
 
-static const char rcsid[] = RCSID;
 
 eap_state eap_states[NUM_PPP]; /* EAP state; one for each unit */
 #ifdef USE_SRP
diff --git a/pppd/ecp.c b/pppd/ecp.c
index e5754e5..dada8e6 100644
--- a/pppd/ecp.c
+++ b/pppd/ecp.c
@@ -59,7 +59,6 @@
 
 #define RCSID  "$Id: ecp.c,v 1.4 2004/11/04 10:02:26 paulus Exp $"
 
-static const char rcsid[] = RCSID;
 
 #include 
 
diff --git a/pppd/eui64.c b/pppd/eui64.c
index d025eff..e7be0e1 100644
--- a/pppd/eui64.c
+++ b/pppd/eui64.c
@@ -39,7 +39,6 @@
 
 #include "pppd.h"
 
-static const char rcsid[] = RCSID;
 
 /*
  * eui64_ntoa - Make an ascii representation of an interface identifier
diff --git a/pppd/fsm.c b/pppd/fsm.c
index e9bd34f..d78ef79 100644
--- a/pppd/fsm.c
+++ b/pppd/fsm.c
@@ -55,7 +55,6 @@
 #include "pppd.h"
 #include "fsm.h"
 
-static const char rcsid[] = RCSID;
 
 static void fsm_timeout __P((void *));
 static void fsm_rconfreq __P((fsm *, int, u_char *, int));
diff --git a/pppd/ipcp.c b/pppd/ipcp.c
index e9738fe..e47ca78 100644
--- a/pppd/ipcp.c
+++ b/pppd/ipcp.c
@@ -61,7 +61,6 @@
 #include "ipcp.h"
 #include "pathnames.h"
 
-static const char rcsid[] = RCSID;
 
 /* global vars */
 ipcp_options ipcp_wantoptions[NUM_PPP];/* Options that we want to 
request */
diff --git a/pppd/ipv6cp.c b/pppd/ipv6cp.c
index 356ff84..f9badc1 100644
--- a/pppd/ipv6cp.c
+++ b/pppd/ipv6cp.c
@@ -168,7 +168,6 @@
 #include "magic.h"
 #include "pathnames.h"
 
-static const char rcsid[] = RCSID;
 
 /* global vars */
 ipv6cp_options ipv6cp_wantoptions[NUM_PPP]; /* Options that we want to 
request */
diff --git a/pppd/ipxcp.c b/pppd/ipxcp.c
index aaff10f..04605b1 100644
--- a/pppd/ipxcp.c
+++ b/pppd/ipxcp.c
@@ -62,7 +62,6 @@
 #include "pathnames.h"
 #include "magic.h"
 
-static const char rcsid[] = RCSID;
 
 /* global vars */
 ipxcp_options ipxcp_wantoptions[NUM_PPP];  /* Options that we want to 
request */
diff --git a/pppd/lcp.c b/pppd/lcp.c
index 8ed2778..625d2f7 100644
--- a/pppd/lcp.c
+++ b/pppd/lcp.c
@@ -56,7 +56,6 @@
 #include "chap-new.h"
 #include "magic.h"
 
-static const char rcsid[] = RCSID;
 
 /*
  * When the link comes up we want to be able to wait for a short while,
diff --git a/pppd/magic.c b/pppd/magic.c
index 3f3ce32..e8bb71f 100644
--- a/pppd/magic.c
+++ b/pppd/magic.c
@@ -51,7 +51,6 @@
 #include "pppd.h"
 #include "magic.h"
 
-static const char rcsid[] = RCSID;
 
 extern long mrand48 (void);
 extern void srand48 (long);
diff --git a/pppd/main.c b/pppd/main.c
index e09b6ff..b0d772b 100644
--- a/pppd/main.c
+++ b/pppd/main.c
@@ -121,7 +121,6 @@
 #include "atcp.h"
 #endif
 
-static const char rcsid[] = RCSID;
 
 /* interface vars */
 char ifname[MAXIFNAMELEN]; /* Interface name */
diff --git a/pppd/options.c b/p

[PATCH 9/9] pppd: refactor setjmp/longjmp with pipe pair in event wait loop

[Kurt Van Dijck]

setjmp/longjmp isn't supported by all compilers.
Having a pipe pair to wake an event wait loop from within a signal handler
is rather portable and common enough.

Signed-off-by: Kurt Van Dijck 
---
 pppd/main.c | 41 +
 pppd/tty.c  |  1 -
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/pppd/main.c b/pppd/main.c
index b0d772b..41be532 100644
--- a/pppd/main.c
+++ b/pppd/main.c
@@ -80,7 +80,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -180,7 +179,7 @@ int got_sighup;
 
 static sigset_t signals_handled;
 static int waiting;
-static sigjmp_buf sigjmp;
+static int sigpipe[2];
 
 char **script_env; /* Env. variable values for scripts */
 int s_env_nalloc;  /* # words avail at script_env */
@@ -598,19 +597,21 @@ static void
 handle_events()
 {
 struct timeval timo;
+unsigned char buf[16];
 
 kill_link = open_ccp_flag = 0;
-if (sigsetjmp(sigjmp, 1) == 0) {
-   sigprocmask(SIG_BLOCK, _handled, NULL);
-   if (got_sighup || got_sigterm || got_sigusr2 || got_sigchld) {
-   sigprocmask(SIG_UNBLOCK, _handled, NULL);
-   } else {
-   waiting = 1;
-   sigprocmask(SIG_UNBLOCK, _handled, NULL);
-   wait_input(timeleft());
-   }
-}
+
+/* alert via signal pipe */
+waiting = 1;
+/* flush signal pipe */
+for (; read(sigpipe[0], buf, sizeof(buf)) > 0; );
+add_fd(sigpipe[0]);
+/* wait if necessary */
+if (!(got_sighup || got_sigterm || got_sigusr2 || got_sigchld))
+   wait_input(timeleft());
 waiting = 0;
+remove_fd(sigpipe[0]);
+
 calltimeout();
 if (got_sighup) {
info("Hangup (SIGHUP)");
@@ -645,6 +646,14 @@ setup_signals()
 {
 struct sigaction sa;
 
+/* create pipe to wake up event handler from signal handler */
+if (pipe(sigpipe) < 0)
+   fatal("Couldn't create signal pipe: %m");
+fcntl(sigpipe[0], F_SETFD, fcntl(sigpipe[0], F_GETFD) | FD_CLOEXEC);
+fcntl(sigpipe[1], F_SETFD, fcntl(sigpipe[1], F_GETFD) | FD_CLOEXEC);
+fcntl(sigpipe[0], F_SETFL, fcntl(sigpipe[0], F_GETFL) | O_NONBLOCK);
+fcntl(sigpipe[1], F_SETFL, fcntl(sigpipe[1], F_GETFL) | O_NONBLOCK);
+
 /*
  * Compute mask of all interesting signals and install signal handlers
  * for each.  Only one signal handler may be active at a time.  Therefore,
@@ -1431,7 +1440,7 @@ hup(sig)
kill_my_pg(sig);
 notify(sigreceived, sig);
 if (waiting)
-   siglongjmp(sigjmp, 1);
+   write(sigpipe[1], , sizeof(sig));
 }
 
 
@@ -1452,7 +1461,7 @@ term(sig)
kill_my_pg(sig);
 notify(sigreceived, sig);
 if (waiting)
-   siglongjmp(sigjmp, 1);
+   write(sigpipe[1], , sizeof(sig));
 }
 
 
@@ -1466,7 +1475,7 @@ chld(sig)
 {
 got_sigchld = 1;
 if (waiting)
-   siglongjmp(sigjmp, 1);
+   write(sigpipe[1], , sizeof(sig));
 }
 
 
@@ -1501,7 +1510,7 @@ open_ccp(sig)
 {
 got_sigusr2 = 1;
 if (waiting)
-   siglongjmp(sigjmp, 1);
+   write(sigpipe[1], , sizeof(sig));
 }
 
 
diff --git a/pppd/tty.c b/pppd/tty.c
index c9a0b33..7ece675 100644
--- a/pppd/tty.c
+++ b/pppd/tty.c
@@ -83,7 +83,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-- 
1.8.5.rc3