Add tcgetwinsize() and tcsetwinsize() to termios.h

[0.gangzta_gmail.com]

Summary

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251868

This patch is for adding the tcgetwinsize() and tcsetwinsize() functions that are expected to be a part of POSIX.1 issue 8: https://www.austingroupbugs.net/view.php?id=1151#c3856. These functions get/set tty winsize respectively. They are currently available in NetBSD and in musl libc.

If this patch is accepted, then

1. please use "soumendraganguly AT gmail DOTCOM" for my email address in the commit;
2. I will happily create documentation for tcgetwinsize(), tcsetwinsize()!

Test Plan

test.c820 BDownload

[0.gangzta_gmail.com]

sys/ttycom.h contains the definition of struct winsize needed for tcgetwinsize() and tcsetwinsize(); I have moved it to the protected block right before the tcgetwinsize() and the tcsetwinsize() declarations. Please compare that with the NetBSD header: https://github.com/NetBSD/src/blob/trunk/sys/sys/termios.h. In fact, both the NetBSD and the OpenBSD versions of termios.h only have sys/ttydefaults.h in the last block after the #endif /* !_TERMIOS_H_ */:

https://github.com/NetBSD/src/blob/trunk/sys/sys/termios.h
https://github.com/openbsd/src/blob/master/sys/sys/termios.h

Glibc does something similar; see https://github.com/bminor/glibc/blob/master/termios/termios.h. However, Glibc's sys/ttydefaults.h is being included in the protected block. FreeBSD's termios.h was changed from https://svnweb.freebsd.org/base/head/include/termios.h?revision=191882&view=markup to https://svnweb.freebsd.org/base/head/include/termios.h?revision=199898&view=markup when ttycom.h suddenly got moved to the last block; there is no explanation in the SVN "commit message" as to why that was done. I suspect that it was an unintentional/accidental change, unless I am missing something that is unique to FreeBSD in this case.

[yuripv]

include/termios.h

99

The proposal says struct winsize is to be added to termios.h, so this include should not be needed, struct winsize define can live here under the same check for new __POSIX_VISIBLE?

100

Wonder if we need new __POSIX_VISIBLE value for issue 8, to put these functions under.

lib/libc/gen/termios.c

54

Gratuitous change; empty line is there per style(9), for functions without local variables.

[0.gangzta_gmail.com]

include/termios.h

99

Great question! The POSIX manpage [ https://pubs.opengroup.org/onlinepubs/9699919799/ ] says that struct termios must be defined in termios.h. However, the FreeBSD termios.h does that via the #include <sys/_termios.h>. Glibc also has several different versions of struct termios based on platform; they each lie in a platform-specific header which is then included in termios.h; for example https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/bits/termios-struct.h, https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/alpha/bits/termios-struct.h, https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/sparc/bits/termios-struct.h etc. Currently we have to include sys/ioctl.h for struct winsize; sys/ioctl.h includes sys/ttycom.h. If we define struct winsize here directly, then all programs that include both termios.h and sys/ioctl.h will break because of multiple definition. According to the POSIX proposal, including termios.h should be enough to get struct winsize defined; however, for backward compatibility, sys/ioctl.h must also include a definition of struct winsize. This problem is solved by including sys/ttycom.h. Note: NetBSD https://github.com/NetBSD/src/blob/trunk/sys/sys/termios.h includes sys/ttycom.h for struct winsize.

100

Just for reference: NetBSD is keeping the sys/ttycom.h, tcgetwinsize(), and tcsetwinsize() includes/declarations in termios.h outside of all POSIX_VISIBLE/BSD_VISIBLE protection. Should I do that instead? After POSIX.1 issue 8 is released, we can then define a new __POSIX_VISIBLE if necessary.

lib/libc/gen/termios.c

54

I was unaware of style(9). I will also add empty lines in tcgetwinsize() and tcsetwinsize() in that case.

[0.gangzta_gmail.com]

Restored empty lines in tcgetattr() and introduced empty lines in tcgetwinsize(), tcsetwinsize() as suggested by @yuripv [ style(9) ].

In termios.h, moved

#include <sys/ttycom.h>
int	tcgetwinsize(int, struct winsize *);
int	tcsetwinsize(int, const struct winsize *);

out of the #if __BSD_VISIBLE block.

[0.gangzta_gmail.com]

@kib I moved the patches here from Bugzilla. I have also updated them. Can you please take another look at them?

[yuripv]

include/termios.h

99

If we define struct winsize here directly, then all programs that include both termios.h and sys/ioctl.h will break because of multiple definition.

Then place it under the same __POSIX_VISIBLE and #ifndef guards?

[0.gangzta_gmail.com]

include/termios.h

99

Are you referring to moving #include <sys/ttycom.h> to within the __POSIX_VISIBLE block? Or define struct winsize within those guards? I might not have been able to convey the message properly earlier: when the POSIX proposal says that struct winsize must be defined in termios.h, I think they mean that including termios.h should be enough to get struct winsize defined in your program; the definition does not have to literally live in that file; if that is not the case, then FreeBSD is already violating POSIX by not putting the definition of struct termios directly in termios.h, because the current POSIX standard https://pubs.opengroup.org/onlinepubs/9699919799/ states that termios.h must include a definition of struct termios. Also, consider the case of Glibc, which has several different versions of struct termios based on platform [ I gave links earlier ]; putting them all in one file and guarding them using #ifdefs will make the file unreadable. I can, however, move the #include <sys/ttycom.h>, tcgetwinsize(), and the tcsetwinsize() declarations in the __POSIX_VISIBLE block if you wish.

[0.gangzta_gmail.com]

Sorry here is the exact link to the current termios.h POSIX specification: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/termios.h.html. Please note the line "The <termios.h> header shall define the termios structure, which shall include at least the following members..."; then note that the FreeBSD termios.h does not directly define struct termios; instead, it imports the definition from #include <sys/_termios.h>.

[0.gangzta_gmail.com]

@yuripv I have added you to the reviewer list.

[0.gangzta_gmail.com]

Another note: won't defining the same struct [ struct winsize ] in 2 different places [ sys/ttycom.h and termios.h ] just make it harder to maintain in case we need to modify the definition in the future? sys/ttycom.h already provides appropriate guards that will prevent multiple definition in case one's program includes both termios.h and sys/ioctl.h.

[0.gangzta_gmail.com]

Straightforward question: Why does struct winsize have to be defined explicitly in this file even though struct termios does not? The CURRENT POSIX specs demand that struct termios be defined in termios.h. Someone please answer this. If this is answered, then I will not hesitate to explicitly define struct winsize here anymore, even though redundant definitions in various files instead of a central location [ sys/ttycom.h ] seems like bad practice.

[0.gangzta_gmail.com]

To further strengthen my argument, I would like to point out that since our tcgetwinsize() and tcsetwinsize() are using ioctl()+TIOCGWINSZ/TIOCSWINSZ to implement tcgetwinsize()/tcsetwinsze(), our struct winsize must be the same as the one being used by sys/ioctl.h, which is included from sys/ttycom.h. Also, like NetBSD, musl libc does not directly define struct winsize in their termios.h either; see https://git.musl-libc.org/cgit/musl/tree/include/termios.h; they are defining it by including <bits/alltypes.h>. Sorry for playing devil's advocate, but I want to make sure that I learn things right: why should we place the tcgetwinsize(), tcsetwinsize() declarations under __POSIX_VISIBLE when they are not a part of POSIX *yet*?

[kib]

Learn what namespaces are, and why we try not to break them. Your patch is incorrect, both in prototyping tc*etwinsize() without scope, and contaminating posix-mandated header and user namespace by including implementation-specific headers.

It would take much less efforts to do it right than write all texts you already did there. Following _termios.h example solves around half of the issues with the patch, another half is the scope for functions declarations.

lib/libc/gen/Symbol.map

426

The symbols list should be alphabetically ordered.

[0.gangzta_gmail.com]

@kib Beautiful! Thank you for the explanation :D I will do my homework right away.

[0.gangzta_gmail.com]

Removes #include <sys/ttycom.h> from termios.h to prevent including unnecessary definitions in the posix header. Since struct winsize is still needed, it is now separately defined in sys/_termios.h with appropriate guards added.

[0.gangzta_gmail.com]

include/termios.h

101

Should I undo this removal?

[kib]

sys/sys/_termios.h

230 ↗

(On Diff #81027)

This is nonsense, winsize is not in POSIX 2001.

sys/sys/ttycom.h

51

The structure definition should go somewhere in sys/sys/_winsize.h. Include file should be used instead of copy-pasting, its guards provide protection against redefinition.

I pointed to _termios.h for this reason, not to contaminate _termios.h with unrelated stuff that leaks into namespace.

[0.gangzta_gmail.com]

sys/sys/_termios.h

230 ↗

(On Diff #81027)

That was a dumb choice on my part. Incredibly sorry about that. @kib @yuripv it looks like POSIX issue 8 will be out in 2022. Meanwhile, may I keep the winsize related material outside the __POSIX_VISIBLE guards?

sys/sys/ttycom.h

51

Working on it! Thank you for the pointers. I am actually learning a ton from this one set of examples.

[kib]

sys/sys/_termios.h

230 ↗

(On Diff #81027)

It should be under BSD_VISIBLE namespace guard, until POSIX defines it and we can add || POSIX_VISIBLE >= <posix version>.

[0.gangzta_gmail.com]

sys/sys/_termios.h

230 ↗

(On Diff #81027)

Gotcha.

[0.gangzta_gmail.com]

The winsize related material should currently be under __BSD_VISIBLE guard.

[0.gangzta_gmail.com]

I have made the requested changes. Notes:

1. I restored the #include <sys/ttycom.h> in the last block of termios.h. However, I am still not sure if that should go in the _TERMIOS_H guarded block because that's how it was before it was changed in 2009 [ from https://svnweb.freebsd.org/base/head/include/termios.h?revision=191882&view=markup to https://svnweb.freebsd.org/base/head/include/termios.h?revision=199898&view=markup ]. No other platform is doing this. Anyway, I understand now that no matter what, the sys/ttycom.h include should be a part of the __BSD_VISIBLE namespace.
2. The copyright notice in sys/_winsize.h might require modification.

[kib]

Surprisingly, it looks fine. I will spend more time with the patch shortly.

sys/sys/_winsize.h

31

You manually edited SCCS line, it does not make sense. Remove it instead.

[0.gangzta_gmail.com]

Removed SCCS line.

[0.gangzta_gmail.com]

In D27650#620577, @kib wrote:

Surprisingly, it looks fine.

Haha. Well, at least now I understand some things better! The situation is a little strange because I am having to do the whole thing on Debian GNU/Linux. Only one time did I test in a FreeBSD vm to build the libc; that's when I realized that I needed the linker symbol file; I had to grep for an hour to figure out that. Sorry for the inconvenience. OTOH, while it's not a big deal anymore, I wonder why some of the other platforms such as NetBSD have chosen to not put the winsize materials under _NETBSD_SOURCE.

Some context: I am attempting to fix cpython's pty library, which has not received a major update in ~2 decades; currently pty.spawn() hangs on the BSDs, macOS, Solaris, etc. On top of that, there are other problems such as not setting winsize, termios, etc. I decided that if I had to fix the problem, I should do it correctly, and making sure that we have these functions standardized on my favorite platforms is step #1 :)

Thank you @kib, @yuripv for your time. Have a merry Christmas if that is one of the holidays that you celebrate :)

[0.gangzta_gmail.com]

sys/sys/_winsize.h

3

Following some other patches, I added my name here. Please let me know if this is acceptable.

[kib]

sys/sys/_winsize.h

3

There is no single line of your code there, perhaps except the include guards.

[0.gangzta_gmail.com]

Updated license.

[0.gangzta_gmail.com]

sys/sys/_winsize.h

3

Aah. I get it. Since the code was taken from sys/ttycom.h, sys/_winsize.h now has the same copyright message as the former. Does that work?

[kib]

You promised to work on the man page updates. Note that FreeBSD project has an approval from Austin Group to use POSIX specification texts as the baseline for the man pages. You can copy/paste most of the text from the AG ticket, I believe.

[0.gangzta_gmail.com]

@kib I will definitely keep my promise. May I please have until the end of the weekend to work on this? I might actually be done way before that, in which case I will submit it immediately after I am done.

[0.gangzta_gmail.com]

In D27650#620599, @0.gangzta_gmail.com wrote:

OTOH, while it's not a big deal anymore, I wonder why some of the other platforms such as NetBSD have chosen to not put the winsize materials under _NETBSD_SOURCE.

Here is the answer to the question:

https://mail-index.netbsd.org/tech-userlevel/2017/10/21/msg010899.html

Basically, Christos Zoulas cleverly avoided the namespace contamination by letting sys/ttycom.h only make visible struct winsize when _NETBSD_SOURCE is not defined:

https://github.com/NetBSD/src/blob/trunk/sys/sys/ttycom.h