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

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

[Kurt Van Dijck]

Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
---
 include/net/ppp_defs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/ppp_defs.h b/include/net/ppp_defs.h
index b06eda5..ed04486 100644
--- a/include/net/ppp_defs.h
+++ b/include/net/ppp_defs.h
@@ -35,6 +35,8 @@
  * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <time.h>
+
 #ifndef _PPP_DEFS_H_
 #define _PPP_DEFS_H_
 
-- 
1.8.5.rc3

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

[Paul Mackerras]

On Thu, Sep 26, 2019 at 09:21:01AM +0200, Kurt Van Dijck wrote:
> Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> ---
>  include/net/ppp_defs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/net/ppp_defs.h b/include/net/ppp_defs.h
> index b06eda5..ed04486 100644
> --- a/include/net/ppp_defs.h
> +++ b/include/net/ppp_defs.h
> @@ -35,6 +35,8 @@
>   * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +#include <time.h>
> +
>  #ifndef _PPP_DEFS_H_
>  #define _PPP_DEFS_H_

I applied this series, but then reverted this one because it breaks
compilation of the kernel device driver on Solaris.  What exactly is
the error that you are seeing without this #include?  Would your error
be fixed by including <sys/time.h> (which would be OK on Solaris)?

Paul.

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

[Kurt Van Dijck]

On vr, 04 okt 2019 08:40:54 +1000, Paul Mackerras wrote:
> On Thu, Sep 26, 2019 at 09:21:01AM +0200, Kurt Van Dijck wrote:
> > Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > ---
> >  include/net/ppp_defs.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/net/ppp_defs.h b/include/net/ppp_defs.h
> > index b06eda5..ed04486 100644
> > --- a/include/net/ppp_defs.h
> > +++ b/include/net/ppp_defs.h
> > @@ -35,6 +35,8 @@
> >   * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >   */
> >  
> > +#include <time.h>
> > +
> >  #ifndef _PPP_DEFS_H_
> >  #define _PPP_DEFS_H_
> 
> I applied this series, but then reverted this one because it breaks
> compilation of the kernel device driver on Solaris.  What exactly is
> the error that you are seeing without this #include?  Would your error
> be fixed by including <sys/time.h> (which would be OK on Solaris)?

I have no Solaris around, hence I didn't see that problem :-)

My output:

cd pppd; make  all
make[1]: Entering directory `/srv/src/net/ppp/pppd'
musl-gcc -O2 -pipe -Wall -g -DHAVE_PATHS_H -DIPX_CHANGE -DHAVE_MMAP -I../include '-DDESTDIR="/usr/local"' -DCHAPMS=1 -DMPPE=1 -DHAS_SHADOW -DHAVE_CRYPT_H=1 -I/usr/include/openssl -DHAVE_MULTILINK -DUSE_TDB=1 -DPLUGIN -DINET6=1 -DMAXOCTETS   -c -o sha1.o sha1.c
In file included from sha1.c:21:0:
../include/net/ppp_defs.h:182:5: error: unknown type name ‘time_t’
     time_t xmit_idle;  /* time since last NP packet sent */
     ^
../include/net/ppp_defs.h:183:5: error: unknown type name ‘time_t’
     time_t recv_idle;  /* time since last NP packet received */
     ^
make[1]: *** [sha1.o] Error 1
make[1]: Leaving directory `/srv/src/net/ppp/pppd'

the time_t type isn't defined while being used.
AFAIK, time_t is defined in time.h

Wrapping the include inside a #ifdef SOLARIS seems the most correct solution.
including sys/time.h works, but that is rather by accident at this moment.

Kind regards,
Kurt

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

[Levente]

IMHO time_t is defined in sys/types.h


Lev

On Fri, Oct 4, 2019 at 9:28 AM Paul Mackerras <paulus@ozlabs.org> wrote:
>
> On Thu, Sep 26, 2019 at 09:21:01AM +0200, Kurt Van Dijck wrote:
> > Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > ---
> >  include/net/ppp_defs.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/net/ppp_defs.h b/include/net/ppp_defs.h
> > index b06eda5..ed04486 100644
> > --- a/include/net/ppp_defs.h
> > +++ b/include/net/ppp_defs.h
> > @@ -35,6 +35,8 @@
> >   * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >   */
> >
> > +#include <time.h>
> > +
> >  #ifndef _PPP_DEFS_H_
> >  #define _PPP_DEFS_H_
>
> I applied this series, but then reverted this one because it breaks
> compilation of the kernel device driver on Solaris.  What exactly is
> the error that you are seeing without this #include?  Would your error
> be fixed by including <sys/time.h> (which would be OK on Solaris)?
>
> Paul.

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

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

[James Carlson]

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.

There may be systems on which including <time.h> 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.

Do you know of a system where either (a) <sys/time.h> 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.

-- 
James Carlson         42.703N 71.076W         <carlsonj@workingcode.com>

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 <time.h> 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) <sys/time.h> 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

[James Carlson]

On 10/04/19 10:29, Kurt Van Dijck wrote:
> 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.

I didn't just go trolling a grepping for time_t.  sys/time.h is pretty
well-established in UNIX, and I think you're punting when you point to
ANSI C alone as the authority here.

As for documentation, how does SUSv2 seem?

https://pubs.opengroup.org/onlinepubs/7908799/xsh/systime.h.html

> 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 <time.h> 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.

-- 
James Carlson         42.703N 71.076W         <carlsonj@workingcode.com>