| Message ID | 20230604204258.2026816-2-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | O_IGNORE_CTTY everywhere | expand |
On 04/06/23 17:42, Sergey Bugaev via Libc-alpha wrote: > This internal definition makes it possible to use O_IGNORE_CTTY in > the glibc codebase unconditionally, no matter whether the current port > provides it or not (i.e. both on Hurd and on Linux). Along with the > definition, this adds a small guide on when O_IGNORE_CTTY is to be used. > > The following commit will actually make use of O_IGNORE_CTTY > throughout the glibc codebase. > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > --- > include/fcntl.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/include/fcntl.h b/include/fcntl.h > index be435047..d788db2e 100644 > --- a/include/fcntl.h > +++ b/include/fcntl.h > @@ -33,6 +33,21 @@ extern int __openat_2 (int __fd, const char *__path, int __oflag); > extern int __openat64_2 (int __fd, const char *__path, int __oflag); > > > +/* Makes open () & friends faster on the Hurd, but can only be used (without > + altering user-visible behavior) when we're sure that the file we're opening > + is not (at the moment) our controlling terminal. Use this when: > + - opening well-known files internally (utmp, nss db); > + - opening files with user-specified names that can not reasonably be ttys > + (sem_open, shm_open); > + - opening new (previously unused) ttys (openpty). > + Don't use this when: > + - doing a general-purpose open () with a user-controlled path that could > + well be "/dev/tty" (fopen). */ > +#ifndef O_IGNORE_CTTY > +# define O_IGNORE_CTTY 0 > +#endif > + I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which defines O_IGNORE_CTTY unconditionally and include the default one (either directly or though include_next.h). We currently are trying to avoid the "#ifdef ...", so a code that does not define, where is should, would fail at compile time. > + > #if IS_IN (rtld) > # include <dl-fcntl.h> > #endif
Hello, On Mon, Jun 5, 2023 at 9:25 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > We currently are trying to avoid the > "#ifdef ...", so a code that does not define, where is should, would fail > at compile time. Yes, this makes perfect sense, and it was something I was also slightly concerned about (what if the Hurd's real definition stops being brought in by include/fcntl.h for some reason? -- then we'd just silently get a 0, and nobody would notice). On the other hand I wanted to not cause any additional troubles for other potential ports (FreeBSD), but maybe it's fine to require them to just add their own little header. Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY and not to 0? > I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which > defines O_IGNORE_CTTY unconditionally and include the default one (either > directly or though include_next.h). Could you please clarify how this whole system of file overrides works? (I mean: a more specific sysdep file, for some unclear definition of "specific", automatically overriding a less specific file of the same basename.) I think I've seen vpath get used somewhere, so I would guess that the sysdep (and other) directories are added to vpath order by their priority, and whichever one Make finds first, it passes to the compiler. Header files, I would guess again, are simply handled by passing all the paths (once again properly sorted) with -I, and it's the compiler that looks for the first directory that contains file of the given name -- this makes it possible to #include_next, and this must also be how include/ contains headers that are used during libc compilation but not installed (include/ must not be on the vpath then?). But this brings me to the more specific question: the headers to be installed are also found using vpath during 'make install', right? How would this work, will Make somehow know to not install this sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep installing io/fcntl.h? Sergey
On 09/06/23 06:29, Sergey Bugaev wrote: > Hello, > > On Mon, Jun 5, 2023 at 9:25 PM Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >> We currently are trying to avoid the >> "#ifdef ...", so a code that does not define, where is should, would fail >> at compile time. > > Yes, this makes perfect sense, and it was something I was also > slightly concerned about (what if the Hurd's real definition stops > being brought in by include/fcntl.h for some reason? -- then we'd just > silently get a 0, and nobody would notice). On the other hand I wanted > to not cause any additional troubles for other potential ports > (FreeBSD), but maybe it's fine to require them to just add their own > little header. Yeah that's the idea, but by adding a generic one it would be required iff the kernel/system needs to something differente. > > Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY > and not to 0? Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic, so I think we should avoid change the internal open flags in this specific patch. > >> I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which >> defines O_IGNORE_CTTY unconditionally and include the default one (either >> directly or though include_next.h). > > Could you please clarify how this whole system of file overrides > works? (I mean: a more specific sysdep file, for some unclear > definition of "specific", automatically overriding a less specific > file of the same basename.) We have the internal header file, to say 'include/fcntl.h', which is used when building glibc itself including the tests. The internal-only header also includes the installed one, 'io/fcntl.h', which defines the ABI glibc provides. So to override a internal-only definition we have some options: 1. Override the file which is has precedence in the sysdeps selection (which defines the -I at built time). So if you add the file "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included instead of 'include/fcntl.h'. To avoid the need to replicate the same prototypes and definitions on the generic 'include/fcntl.h' in the system specific one we can use the include_next extension (check on how the sysvipc code done, like include/sys/sem.h). 2. Add per-system file that is included in the generic 'include/fcntl.h', for instance fcntl-system.h or something like that. On then add a generic definition on sysdep/generic/ with the expected value and override it on any sysdep folder that requires it. I tend to see the second options as a slight simpler one. > > I think I've seen vpath get used somewhere, so I would guess that the > sysdep (and other) directories are added to vpath order by their > priority, and whichever one Make finds first, it passes to the > compiler. Header files, I would guess again, are simply handled by > passing all the paths (once again properly sorted) with -I, and it's > the compiler that looks for the first directory that contains file of > the given name -- this makes it possible to #include_next, and this > must also be how include/ contains headers that are used during libc > compilation but not installed (include/ must not be on the vpath > then?). > > But this brings me to the more specific question: the headers to be > installed are also found using vpath during 'make install', right? How > would this work, will Make somehow know to not install this > sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep > installing io/fcntl.h? Afaiu there is no need to install any new header for this, this is an internal only definition to use on open call within glibc.
Hello, On Mon, Jun 12, 2023 at 9:56 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY > > and not to 0? > > Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic, > so I think we should avoid change the internal open flags in this > specific patch. OK. > We have the internal header file, to say 'include/fcntl.h', which is > used when building glibc itself including the tests. The internal-only > header also includes the installed one, 'io/fcntl.h', which defines > the ABI glibc provides. Yes, that I understand. > So to override a internal-only definition we have some options: > > 1. Override the file which is has precedence in the sysdeps selection > (which defines the -I at built time). So if you add the file > "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included > instead of 'include/fcntl.h'. Oh cool, I didn't realize it was possible to nest include/ under sysdeps/, thank you! I thought there was only the top-level include/ directory, but now I see that there are more, including even a few Mach/Hurd specific ones. > 2. Add per-system file that is included in the generic 'include/fcntl.h', > for instance fcntl-system.h or something like that. On then add > a generic definition on sysdep/generic/ with the expected value > and override it on any sysdep folder that requires it. Is there already an example of such a header file? I can't find any files in any include/ directory matching either *sysdep* or *system*. > I tend to see the second options as a slight simpler one. The first option sounds simpler to me. Should it be sysdeps/unix/sysv/linux/include/fcntl.h or sysdeps/generic/include/fcntl.h? If it's the latter, I'd have to create a sysdeps/hurd/include/fcntl.h that does not (re)define O_IGNORE_CTTY (right?). > > But this brings me to the more specific question: the headers to be > > installed are also found using vpath during 'make install', right? How > > would this work, will Make somehow know to not install this > > sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep > > installing io/fcntl.h? > > Afaiu there is no need to install any new header for this, this is an > internal only definition to use on open call within glibc. Yes, this is my point: you were suggesting that I add sysdeps/unix/sysv/linux/fcntl.h (notice no /include/ in the middle), which -- I'm not sure whether it would get installed by the build system (instead of io/fcntl.h) or not. I want it to *not* get installed. The option with /include/ in the middle that you're suggesting now should take care of that. Sergey
On 13/06/23 06:42, Sergey Bugaev wrote: > Hello, > > On Mon, Jun 12, 2023 at 9:56 PM Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >>> Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY >>> and not to 0? >> >> Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic, >> so I think we should avoid change the internal open flags in this >> specific patch. > > OK. > >> We have the internal header file, to say 'include/fcntl.h', which is >> used when building glibc itself including the tests. The internal-only >> header also includes the installed one, 'io/fcntl.h', which defines >> the ABI glibc provides. > > Yes, that I understand. > >> So to override a internal-only definition we have some options: >> >> 1. Override the file which is has precedence in the sysdeps selection >> (which defines the -I at built time). So if you add the file >> "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included >> instead of 'include/fcntl.h'. > > Oh cool, I didn't realize it was possible to nest include/ under > sysdeps/, thank you! I thought there was only the top-level include/ > directory, but now I see that there are more, including even a few > Mach/Hurd specific ones. This comes from how the sysdeps (or sysnames from configuration) is organized and used as include directories, it allows system specific path (which as more 'deep' in sysdeps) to take precedence over default ones. > >> 2. Add per-system file that is included in the generic 'include/fcntl.h', >> for instance fcntl-system.h or something like that. On then add >> a generic definition on sysdep/generic/ with the expected value >> and override it on any sysdep folder that requires it. > > Is there already an example of such a header file? I can't find any > files in any include/ directory matching either *sysdep* or *system*. Usually for system specific headers we do not add them on include/ but rather on sysdeps/generic and override when necessary. For instance the sysdeps/generic/math_ldbl.h which is override multiple times because of the multiple long double ABIs we support Another example is the malloc-hugepages.c, which has a common definition but the implementation is reimplemented for Linux. > >> I tend to see the second options as a slight simpler one. > > The first option sounds simpler to me. > > Should it be sysdeps/unix/sysv/linux/include/fcntl.h or > sysdeps/generic/include/fcntl.h? If it's the latter, I'd have to > create a sysdeps/hurd/include/fcntl.h that does not (re)define > O_IGNORE_CTTY (right?). I think for this it would be better to just add a sysdeps/generic/fcntl-system.h (or a better name if you a better idea), and define O_IGNORE_CTTY as 0. The Hurd version will then have a different value. Then include it on include/fcntl.h. Btw, we already do it for dl-fcntl.h to handle another Hurd requirement. > >>> But this brings me to the more specific question: the headers to be >>> installed are also found using vpath during 'make install', right? How >>> would this work, will Make somehow know to not install this >>> sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep >>> installing io/fcntl.h? >> >> Afaiu there is no need to install any new header for this, this is an >> internal only definition to use on open call within glibc. > > Yes, this is my point: you were suggesting that I add > sysdeps/unix/sysv/linux/fcntl.h (notice no /include/ in the middle), > which -- I'm not sure whether it would get installed by the build > system (instead of io/fcntl.h) or not. I want it to *not* get > installed. The option with /include/ in the middle that you're > suggesting now should take care of that. Afaik none of the header on include are installed, each subdir Makefile needs to explicit add the expected file on 'headers' rule. Also, for sysdeps we have an extra rule, sysdep_headers, that adds extra includes.
diff --git a/include/fcntl.h b/include/fcntl.h index be435047..d788db2e 100644 --- a/include/fcntl.h +++ b/include/fcntl.h @@ -33,6 +33,21 @@ extern int __openat_2 (int __fd, const char *__path, int __oflag); extern int __openat64_2 (int __fd, const char *__path, int __oflag); +/* Makes open () & friends faster on the Hurd, but can only be used (without + altering user-visible behavior) when we're sure that the file we're opening + is not (at the moment) our controlling terminal. Use this when: + - opening well-known files internally (utmp, nss db); + - opening files with user-specified names that can not reasonably be ttys + (sem_open, shm_open); + - opening new (previously unused) ttys (openpty). + Don't use this when: + - doing a general-purpose open () with a user-controlled path that could + well be "/dev/tty" (fopen). */ +#ifndef O_IGNORE_CTTY +# define O_IGNORE_CTTY 0 +#endif + + #if IS_IN (rtld) # include <dl-fcntl.h> #endif
This internal definition makes it possible to use O_IGNORE_CTTY in the glibc codebase unconditionally, no matter whether the current port provides it or not (i.e. both on Hurd and on Linux). Along with the definition, this adds a small guide on when O_IGNORE_CTTY is to be used. The following commit will actually make use of O_IGNORE_CTTY throughout the glibc codebase. Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- include/fcntl.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)