Re: [PATCH] time.h: include header before using time_t
On 10/04/19 14:33, Kurt Van Dijck wrote: > On vr, 04 okt 2019 13:52:11 -0400, James Carlson wrote: > 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. I've never heard of this problem. I'm afraid I don't know what you're referring to. I've never heard of a compiler (or other tool chain component) that delivers files to /usr/include/sys. That'd be somewhat surprising to me, but I guess it's a wide world out there. As the name says, the stuff under sys/ is part of the _system_. On UNIX, the standard parts of it are described in the Single UNIX Standard, maintained by The Open Group. That's the documentation pointer I provided previously. Are there systems where system header files aren't installed by default? Sure. That's somewhat commonplace. But on such a machine you can't compile things (including pppd) until you install the (presumably optional) header files. If you look closely, you'll see that pppd/main.c already includes and it's not guarded by any conditional compilation because it's a *STANDARD HEADER FILE*. If there were problems of some sort with this include file, I'd expect they'd have surfaced by now. -- James Carlson 42.703N 71.076W
Re: [PATCH] time.h: include header before using time_t
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
Re: [PATCH] time.h: include header before using time_t
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? -- James Carlson 42.703N 71.076W
[PATCH] time.h: include header before using time_t
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
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 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
Re: [PATCH 4/9] pppd: include time.h before using time_t
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
> 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
IMHO time_t is defined in sys/types.h Lev On Fri, Oct 4, 2019 at 9:28 AM Paul Mackerras wrote: > > On Thu, Sep 26, 2019 at 09:21:01AM +0200, Kurt Van Dijck wrote: > > Signed-off-by: Kurt Van Dijck > > --- > > 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 > > + > > #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 (which would be OK on Solaris)? > > Paul.
